Attention is currently required from: jolly, laforge, dexter.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33511 )
Change subject: ASCI: Add call control for VGCS/VBS ......................................................................
Patch Set 14:
(11 comments)
File src/libmsc/msc_vgcs.c:
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/3eb64441_587e42ec PS14, 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()
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/4dbc4771_95119204 PS14, 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?
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/59a868dd_a420ec16 PS14, 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
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/71b5db22_ffc6a28b PS14, Line 801: if (msc_a) { (early exit pattern: 'if (!msc_a) return;' means less indent. some more candidates for early exit below)
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/e6c8b05f_69d73936 PS14, Line 1628: OSMO_ASSERT(l3_msg); encoding error: do not abort the program, log and return error
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/577b08a5_6e8d079c PS14, Line 1666: OSMO_ASSERT .
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/060c9d51_155c550f PS14, 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).
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/c8814bfa_c1dd2f9a PS14, 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)
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/4c1901cf_986d9645 PS14, Line 1796: OSMO_ASSERT .
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/1186fa77_1bee1bdb PS14, Line 1938: OSMO_ASSERT .
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/c77a36c0_c57383a6 PS14, Line 1949: OSMO_ASSERT .