Attention is currently required from: wbokslag.
laforge 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:
(6 comments)
File src/lower_mac/tetra_lower_mac.c:
https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/c28a8e8e_e37afbcb
PS2, Line 347: msg->len = msg
in general we don't ever manually manipulate msgb's this way in other osmocom
software. We use the various msgb.[ch] functions as high-level operations on the buffer
(like msgb_put/pull/push/get/trim/reserve/...), but don't consider manipulation of the
individual fields as good code.
I don't know the specific use case here (and have too many other tasks to review in
detail) so I don't have a concrete suggestion, just sharing general thoughts.
File src/tetra_llc.c:
https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/e33e4841_ee2edb49
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() ?
File src/tetra_upper_mac.c:
https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/68250cd6_9fecc52e
PS2, Line 178: msg->len = m
same here. msgb_trim?
https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/7b1ffe59_7871231b
PS2, Line 185: msg->len = m
same here. msgb_trim?
https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/edec0a07_0a8865b4
PS2, Line 306: msg->len = m
yet another msgb_trim? Also, the entire get_num_fill_bits + following operations happens
several times in the code, so it might make sense to create a function to cover the
sequence of getting the number of fill-bits and then trimming the message to exclude
those?
https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/46364861_fc916427
PS2, Line 359: msg->len = m
again another fill-bits-stripping situation.
--
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: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Comment-Date: Sat, 29 Jul 2023 08:34:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment