Attention is currently required from: laforge.

wbokslag has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-tetra/+/34000 )

Change subject: Fixups and clarifying comments for msgb tail modifications ......................................................................

Patch Set 2:

(7 comments)

Patchset:

PS2: Hi, thanks again for the extensive comments. I elaborated the issue in a reply on your comment on tetra_lower_mac, and I believe indeed trim / l3trim may be suited for use in tetra_upper_mac and the LLC code

File src/lower_mac/tetra_lower_mac.c:

https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/c0575652_18ca0744 PS2, Line 347: msg->len = msg

in general we don't ever manually manipulate msgb's this way in other osmocom software. […]

Okay, I appreciate the comments and effort to maintain good code quality. I am not deeply familiar with the osmocom primitives, so maybe I'm overlooking something.

The essence of the problem is this. Suppose we have a timeslot on the signalling channel, which will contain MAC-RESOURCE pdus and the like. According to the standard, multiple PDUs may be contained in a single timeslot. So the MAC layer parsing code needs to have an accurate l1h, l2h, l3h start marker for the different layers of the protocol, BUT ALSO need to know how long the payload for their respective layer is. During parsing in tetra_upper_mac, the tail pointer is moved closer to the start, based on MAC-RESOURCE len field. Also, the tail pointer is moved closer to the start in the LLC layer (layer 2) if a 32-bit FCS is present AFTER the layer 3 MLE data. This is needed, because layer 3 parsing requires accurate start (l3h) and end/length markers.

However, once the upper mac parsing is done and we return to the lower mac, we need to parse any further resources that may be present in the timeslot. That's why I advance the head, put the tail at the end of the slot again and then fix the end field.

msgb_get seemed unsuitable for the purpose since its description states "removes data from the end", as such, I felt I have no guarantee the data there will remain intact and/or allocated. Also semantically it seems like we definitively discard this data. In your next comment you bring msgb_trim (for l2) and msgb_l3trim to my attention, that indeed looks suitable, I'll refactor.

File src/tetra_llc.c:

https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/b443b21b_ab95c3cf PS2, Line 124: msg->tail = msg->l3h + lpp.tl_sdu_len; // Strips off FCS (if present) : msg->len = msg->tail - msg->head;

this looks a bit like openn-coding a msgb_get(), or probably even more a msgb_l3trim() ?

Done

File src/tetra_upper_mac.c:

https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/9bdada54_cca44860 PS2, Line 178: msg->len = m

same here. […]

Done

https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/e5e44e08_c7971e2a PS2, Line 185: msg->len = m

same here. […]

Done

https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/1b2939f2_78548fbe PS2, Line 306: msg->len = m

yet another msgb_trim? Also, the entire get_num_fill_bits + following operations happens several ti […]

Ack

https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/c818c925_1fb1ed55 PS2, Line 359: msg->len = m

again another fill-bits-stripping situation.

Done