19 Sep
2022
19 Sep
'22
5:21 p.m.
Attention is currently required from: neels, pespin.
fixeria has posted comments on this change. (
https://gerrit.osmocom.org/c/libosmo-gprs/+/29402 )
Change subject: llc: implement LLC PDU codec based on code from osmo-sgsn.git
......................................................................
Patch Set 2:
(9 comments)
Patchset:
PS2:
Please fix the linter warnings which make sense.
Please see my very first comments above.
File include/osmocom/gprs/llc/llc.h:
https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/9da3c8d9_816dc949
PS2, Line 132:
because it is computationally trivial to do '(1
<< OSMO__CMD_RSP)', but hard to do the reverse (get the bit nr from a bitmask).
Why would you need to do the reverse? I see three most frequent use cases: a) checking if
a flag is set; b) setting a flag; c) unsetting a flag. In all these use cases you need to
do (1 << x), so I find it more convenient to have them defined as bitmasks.
https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/befe6e42_34b5ebce
PS2, Line 135: #define OSMO_GPRS_LLC_PDU_F_ACK_REQ (1 << 2) /* 6.3.5.2
Acknowledgement request bit (A) */
> 'Acknowledgement' may be misspelled -
perhaps 'Acknowledgment'? […]
From my comments above: this is the original
spelling from the specs.
I guess you also saw pag[e]ing in 3GPP docs?
File src/llc/llc_pdu.c:
https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/0fe65358_1b421992
PS2, Line 80: void osmo_gprs_llc_pdu_hdr_dump_buf(const struct osmo_gprs_llc_pdu_decoded
*pdu,
please make this function signature match snprintf(),
because then it can be used by OSMO_STRBUF_APP […]
Ack, will do in the next patch
set.
https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/e0d5d1a2_5145d909
PS2, Line 127: /* 6.4.1 Unnumbered (U) frames */
i'd prefer an enum to group these
What would
be the benefit of doing so?
https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/c02fbaad_d66765ff
PS2, Line 144: addr[0] |= pdu->sapi & 0x0f;
The msgb is passed in from the caller, who knows what
data may linger from a previous msgb_trim() or […]
Ack
https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/5ebc4fd1_fb15ce8c
PS2, Line 157: ctrl[0] |= (pdu->seq_tx >> 4) & 0x1f;
bit shifting would be fine with me; but the advantage
of a packed struct is that the decode function […]
Indeed, this can be improved
later. For now we can live with manual bit-shifts.
https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/0211e025_816a2739
PS2, Line 277: che
weird to see this linter vote after […]
The
linter job has been executing before the mentioned patch was merged.
File tests/llc_pdu_codec/pdu_codec_test.c:
https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/d62ac501_afa7d49b
PS2, Line 66: struct msgb *msg = msgb_alloc(1024, "LLC-PDU");
declare msg at start of scope
... so that the
code looks more like Pascal ;)
What's wrong with doing so (especially in tests)?
--
To view, visit https://gerrit.osmocom.org/c/libosmo-gprs/+/29402
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-gprs
Gerrit-Branch: master
Gerrit-Change-Id: I61d7e2e6d0a8f2cdfc2113e637e447dc428cc70d
Gerrit-Change-Number: 29402
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 19 Sep 2022 15:21:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment