Attention is currently required from: jolly, laforge, dexter.

View Change

11 comments:

• File src/libmsc/msc_vgcs.c:

  • Patch Set #14, Line 134: static char string[9];

    none of these are critical, but i'd still like to mention them:

    Since it is only one format string, it might be implemented as a #define GROUP_ID_FMT "%08u"

    In case of a function,
    i'd use the following pattern because it is very flexible:

     int gsm44068_group_id_str_buf(char *buf, size_t buflen, uint32_t callref)
     {
         struct osmo_strbuf sb = { .buf = buf, .len = buflen };
         OSMO_STRBUF_PRINTF(sb, "%08u", callref);
         return sb.chars_needed;
     }

    • can be used for static alloc
    • can be used for dynamic alloc with OSMO_NAME_C_IMPL
    • can be used to append to arbitrary buffer position directly
    • can be used with OSMO_STRBUF_APPEND()

  • Patch Set #14, Line 179: OSMO_ASSERT(l3_msg);

    this would abort the program on an encoding error?
    rather log and return error.

    These four lines from ran_a_encode to msgb_free seem to duplicate 1:1 many times in this file, put them in a static function?

  • Patch Set #14, Line 231: *callref = ntohl(*(uint32_t *)ie) >> 5;

    maybe better use osmo_load32be() because it also works reliably when the pointer is not on a word boundary

  • Patch Set #14, Line 801: if (msc_a) {

    (early exit pattern: 'if (!msc_a) return;' means less indent. some more candidates for early exit below)

  • Patch Set #14, Line 1628: OSMO_ASSERT(l3_msg);

    encoding error: do not abort the program, log and return error

  • Patch Set #14, Line 1666: OSMO_ASSERT

    .

  • Patch Set #14, Line 1727: osmo_fsm_inst_free(bss->fi);

    rather use osmo_fsm_inst_term(). It terminates the instance gracefully and then frees it.

    (I like avoid doing anything at all after dispatching an event or changing a state to avoid possible use-after-free "loops", but I see BSS_ST_NULL has no onenter() action that could trigger those).

  • Patch Set #14, Line 1730: talloc_free(bss);

    (hint, in case you like it too, i like to allocate the struct as a talloc child of the FSM instance, so an osmo_fsm_inst_term() automatically frees the struct, and they are "atomically" removed at the same time without any race conditions possible)

  • Patch Set #14, Line 1796: OSMO_ASSERT

    .

  • Patch Set #14, Line 1938: OSMO_ASSERT

    .

  • Patch Set #14, Line 1949: OSMO_ASSERT

    .

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