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 . -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33511 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I9947403fde8212b66758104443c60aaacc8b1e7b Gerrit-Change-Number: 33511 Gerrit-PatchSet: 14 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: dexter <pmaier(a)sysmocom.de> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Thu, 13 Jul 2023 00:27:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment