LAPDm code issues (ladpm.c)

[Jacob Erlbeck]

Hello Andreas

On 15.02.2014 10:25, Andreas Eversberg wrote:
> 
> Jacob Erlbeck wrote:
> 
>> - L3 length computation should be done with the macros, how it is done
>> in lapdm_send_ph_data_req() is broken.
> what is actually broken? i see that l3_len is the complete msgb space
> between data and tail.
The line
  uint8_t l3_len = msg->tail - msg->data;
delivers the correct result, that's not the point. But here are 2
different things being mixed:
  - the l3 length definition: l3_len = msg->tail - msg->l3h   (AKA
msgb_l3len())
  - the assumption that msg->data == msg->l3h
Mixing things this way is IMO conceptually broken and hard to maintain.
According to your statement below ("frames that are sent to lower layer
1 should have l2h and l3h set [...]"), l3h must have been set when
lapdm_send_ph_data_req() is entered. The requirement data == l3h on the
other hand is more than requested in general, and it would be helpful if
it was documented and/or asserted.

>> - Why does lapd_msg_ctx have a length field that is not used?
> i guess it was used in early code (when osmocombb project started), but
> it is actually not required.
So it can be removed completely?

>>> - How l2/data/.. in a msg are expected to be used/set should be
>>> documented somewhere.
> each msg has l1h, l2h and l3h pointers. the logical way is (even if it
> is currently not implemented correctly):
> 
> frames that are received from lower layer 1 should have l2h set and may
> have l1 header or not. (layer 2 is not responsible of l1 header, so we
> should ignore it.) l3h must not be set, since layer 1 does not know
> about any l3 header start. the l2_ph_data_ind() function should parse
> the l2h, set l3h and remove everything up to l3h.
> 
> frames that are sent to lower layer 1 should have l2h and l3h set before
> sending to layer 1. see lapdm_send_ph_data_req().

What about putting such definitions somewhere central (e.g. wiki or top
of msgb.h)?


> push the dummy_l1 header after pushing RLL header:
> 
>         msg = msgb_from_array(dummy1, sizeof(dummy1));
>         rsl_rll_push_l3(msg, RSL_MT_DATA_REQ, 0, 0, 1);
> +       msgb_push(msg, dummy_l1len);
> 
> 
> - first the msg is allocated and filled with array. l3h is set.
> - rsl_rll_push_l3 pushes the layer 3 information element and the RLL
> header in front of the msg and sets l2h.
> - at least msgb_push adds the dummy_l1 header in front of everything.
> l1h is not set, but it is not relevant, since l1 header is ignored by
> lapdm.

Thank you, that makes sense.

Jacob