Attention is currently required from: neels, pespin.
9 comments:
Patchset:
Please fix the linter warnings which make sense.
Please see my very first comments above.
File include/osmocom/gprs/llc/llc.h:
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.
Patch Set #2, 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:
Patch Set #2, 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.
Patch Set #2, Line 127: /* 6.4.1 Unnumbered (U) frames */
i'd prefer an enum to group these
What would be the benefit of doing so?
Patch Set #2, 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
Patch Set #2, 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.
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:
Patch Set #2, 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 change 29402. To unsubscribe, or for help writing mail filters, visit settings.