LAPDm code issues (ladpm.c)

[Andreas Eversberg]

hi jacob,

Jacob Erlbeck wrote:
> - The remaining msgb_pull() need to be checked (at least the one in
> l2_ph_data_ind() looks suspicious to me.
the msgb_pull() should actually pull the space between data and l2h 
(after adding 2), but it assumes that there is no l1 header in front. i 
suggest to remove the pull:

- msgb_pull(msg, 2);
   msg->l2h += 2;

no pull, because the space between data and l3h is removed afterwards in 
the same function.

> - 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.
> - 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.
> - ladp_test.c should be extended to check other execution paths, too.
> I've tried it for dummy packets and I didn't get it working without
> failing assertions (see below)
> The following applies to most sub-projects, but I find it hard to
> diagnose and use function calls if their API is not documented beyond
> parameter names and types. Especially when passing a msgb to a function
> it would really help if there was some documentation telling about which
> parts of the msgb are used (as in "l2 is expected to point to the start
> of the XYZ message"):
>> - 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().
> The following patch still breaks the assertions, at least changing the
> l3len computation in lapdm_send_ph_data_req() influences but doesn't fix
> it.
>
> --- a/tests/lapd/lapd_test.c
> +++ b/tests/lapd/lapd_test.c
> @@ -123,8 +123,10 @@ static struct msgb *create_empty_msg(void)
>   static struct msgb *create_dummy_data_req(void)
>   {
>          struct msgb *msg;
> +       const int dummy_l1len = 3;
>
>          msg = msgb_from_array(dummy1, sizeof(dummy1));
> +       msgb_push(msg, dummy_l1len);
>          rsl_rll_push_l3(msg, RSL_MT_DATA_REQ, 0, 0, 1);
>          return msg;
>   }
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.


best regards,

andreas