Attention is currently required from: pespin, fixeria.

View Change

12 comments:

• File include/osmocom/gprs/llc/llc.h:

  • Patch Set #2, 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:

  • Patch Set #1, 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:

  • 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_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;
      }

  • Patch Set #2, 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)
      }

  • Patch Set #2, Line 127: /* 6.4.1 Unnumbered (U) frames */

    i'd prefer an enum to group these

  • 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 other msgb manipulation. So cannot be sure that the msgb is zero initialized. rather make the first assignments '=', not '|='. (same for ctrl[*])

  • Patch Set #2, 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.

  • Patch Set #2, 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)

  • Patch Set #2, Line 277: che

    weird to see this linter vote after
    https://gerrit.osmocom.org/c/osmo-ci/+/29359

  • Patch Set #2, 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".

  • Patch Set #2, Line 326: check_len(sizeof(*ctrl), "missing Control field");

    s/sizeof(*ctrl)/1

• 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

To view, visit change 29402. To unsubscribe, or for help writing mail filters, visit settings.