Change in libosmo-gprs[master]: llc: implement LLC PDU codec based on code from osmo-sgsn.git

Attention is currently required from: pespin, fixeria. neels 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: (12 comments) File include/osmocom/gprs/llc/llc.h: https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/f79d3fd9_4976821e PS2, Line 132: with bitmasks like this it is often much more flexibly useful to define the constants as the shift amount, i.e. enum osmo_gprs_llc_pdu_f { OSMO_..._CMD_RSP = 0, ..._FOLL_FIN = 1, ... } because it is computationally trivial to do '(1 << OSMO__CMD_RSP)', but hard to do the reverse (get the bit nr from a bitmask). Also I'd prefer an enum to group these. File include/osmocom/gprs/llc/llc.h: https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/7f4302bd_72744db8 PS1, Line 135: #define OSMO_GPRS_LLC_PDU_F_ACK_REQ (1 << 2) /* 6.3.5.2 Acknowledgement request bit (A) */ i would always have written "acknowledgement" but apparently the accepted correct spelling is "acknowledgment". i'd be fine with the 'wrong' spelling File src/llc/llc_pdu.c: https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/ad02aeb0_16e80e53 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_APPEND(), as well as OSMO_NAME_C_IMPL to make a foo_dump_c() function: int foo(char *buf, size_t buf_size, const arg) { ... return sb.chars_needed; } https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/e74514e7_f1269c1c PS2, Line 122: static __thread char buf[256]; much prefer to move away from static buffers and instead make this a ctx function char *osmo_gprs_llc_pdu_hdr_dump_c(void *ctx, const ... pdu) { OSMO_NAME_C_IMPL(ctx, 128, "ERROR", osmo_gprs_llc_pdu_hdr_dump_buf, pdu) } https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/51dd925c_51d0031a PS2, Line 127: /* 6.4.1 Unnumbered (U) frames */ i'd prefer an enum to group these https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/056565e4_0260d851 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 other msgb manipulation. So cannot be sure that the msgb is zero initialized. rather make the first assignments '=', not '|='. (same for ctrl[*]) https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/d5d37f59_9f05df90 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 below can use the same struct, hence not "duplicating" the bit shifting code. OTOH if this is code copied 1:1 from osmo-sgsn.git then let's not edit it, just adopt it the way it always was, at least to begin with. https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/3e1cdd8f_3cf8d17e PS2, Line 166: OSMO_ASSERT(pdu->sack.len > 0); hmm, mildly disagree with OSMO_ASSERT() in a library function, would be more polite to return an error code instead (same below) https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/5b756aac_c3b92349 PS2, Line 277: che weird to see this linter vote after https://gerrit.osmocom.org/c/osmo-ci/+/29359 https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/7b27cb6b_8c60834a PS2, Line 293: check_len(sizeof(*addr), "missing Address field"); addr being a uint8_t*, sizeof(*addr) is a very elaborate and possibly confusing way to write "1". Please just write "1". https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/faca719d_0638b9e6 PS2, Line 326: check_len(sizeof(*ctrl), "missing Control field"); s/sizeof(*ctrl)/1 File tests/llc_pdu_codec/pdu_codec_test.c: https://gerrit.osmocom.org/c/libosmo-gprs/+/29402/comment/39befa3a_64d3cce6 PS2, Line 66: struct msgb *msg = msgb_alloc(1024, "LLC-PDU"); declare msg at start of scope -- 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 &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-CC: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-CC: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Comment-Date: Mon, 19 Sep 2022 11:41:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-MessageType: comment