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