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)?