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