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) */
No, this is how it's written in the specs.
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;
looks like we want a packed struct for "ctrl" here?
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