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
--
To view, visit
https://gerrit.osmocom.org/c/osmo-tetra/+/34000
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-tetra
Gerrit-Branch: master
Gerrit-Change-Id: Ia725edbeafe26bd2ea9b5a1810d0b26bc79d84db
Gerrit-Change-Number: 34000
Gerrit-PatchSet: 2
Gerrit-Owner: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Sat, 29 Jul 2023 09:47:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: comment