Attention is currently required from: laforge.
7 comments:
Patchset:
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:
Patch Set #2, 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:
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:
Patch Set #2, Line 178: msg->len = m
same here. […]
Done
Patch Set #2, Line 185: msg->len = m
same here. […]
Done
Patch Set #2, Line 306: msg->len = m
yet another msgb_trim? Also, the entire get_num_fill_bits + following operations happens several ti […]
Ack
Patch Set #2, Line 359: msg->len = m
again another fill-bits-stripping situation.
Done
To view, visit change 34000. To unsubscribe, or for help writing mail filters, visit settings.