Attention is currently required from: jolly. laforge 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 10: (15 comments) File src/libmsc/vgcs_fsm.c: https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/5f7b6d12_9b45ad97 PS10, Line 123: tatic __attribute__((constructor)) void vgcs_bcc_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_bcc_fsm) == 0); : } : : static __attribute__((constructor)) void vgcs_gcc_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_gcc_fsm) == 0); : } : : static __attribute__((constructor)) void vgcs_bss_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_bss_fsm) == 0); : } : : static __attribute__((constructor)) void vgcs_cell_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_cell_fsm) == 0); : } : : static __attribute__((constructor)) void vgcs_mgw_ep_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_mgw_ep_fsm) == 0); : } is there a reason to have separate constructor symbols for this? Why not have one consturctor function which has N OSMO_ASSERT statements? https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/5676916c_34b24ae6 PS10, Line 152: sprintf(string, "%08u", callref); yes, 8 decimal digits plus zero termination is shorter than 16 characters, but I'd say as a matter of good practice we should still use snprintf. This makes it easier for somebody else to to audit the code, where every sprintf requires auditing if it's really ok, with snprintf you can just ignore it from manual audit/inspection. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/f5c4c9e6_54602382 PS10, Line 222: if (with_prio) also here I would first build the 32bit value as a stack variable of uint32_t type and use msgb_put_u32() to avoid having to type-cast around and using the third byte of an uint32.... https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/96efd94e_f92b21a9 PS10, Line 232: int _ie_invalid(void) are those (_add_cause_ie, _add_callref_ie, _msg_too_short, _ie_invalid) really intended to be exported (non-static) functions? IF yes, then their name should be expanded to reflect they relate to ASCI as the name is very geneirc. If not, then they should be static. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/436e05d4_dca1bc9d PS10, Line 249: e = msgb_put(msg, 1) you can first buld the uint8_t as a local variable and then use msgb_put_u8(msg, value), avoiding a pointer to an array and indexing only the 0th element in it. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/48de5e4a_926d7567 PS10, Line 265: #if not sure why this is commented out? because there are no users yet and it's a static function? IF there is a good reason to do it like this, then add a comment as explanation, please. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/4915c3cf_455819db PS10, Line 314: payload_len as you're decrmenting this variable, I thing I'd call it remaining_len or remaining_payload_len or something like that. payload_len sounds like the total length of the payloda field https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/1d80b9d4_bcf8d0f6 PS10, Line 323: [0] + 1; I don't have an immediate better solution, but I really think ti is very hard to read/follow and particularly had to say if it is correct, contains some overflows, .... You're basically incrementing the ie pointer as you go, but also deducting the payload_len pointer, both by the same amount, causing duplicate sub-expressions like "ie[0] + 1" which all must be consistent. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/7c636d95_5af9fece PS10, Line 337: ie[0] + here I think you're trusting ie[0] blindly, without having done much checking on it? probably osmo_mobile_identity_decode() imlpicitly trusts it as it is used as input variable. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/33b80962_fc054624 PS10, Line 346: ie += 4; in each of those blocks we have a magic value (length) that is used three times and which must be consistent. 4 here in this example. I think there must be a way without all those copies of the same value. Like some kind of helper functions, or in the worst case macros? https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/a6d317a3_034b52be PS10, Line 391: ie[0] = (da << 3) again I'd put the value together in a uint8_t local variable and then msgb_put_u8() it aftrewards. This way we're not de-refernecing the zero'th element in an aray of 1, which makes it harder to read (and one might assume the array might have more elements, ... https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/5c2a76a1_90ecbadf PS10, Line 416: if (ie[3] & 0x10) { same comments as other decoders above. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/475f3568_92b349ae PS10, Line 604: osmo_fsm_inst_free(mgw->fi); we typically use the goto err_* label construct here to not have to repeat all the various free functions in every case. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/e7774254_19940618 PS10, Line 694: return -EINVAL; here we return -EINVAL without talloc_free etc? https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/6dbde9df_e5b6777e PS10, Line 1279: int gsm44068_rcv_bcc_gcc(struct msc_a *msc_a, struct gsm_trans *trans, struct msgb *msg) this function is quite long. Even the variable list alone fills half a page on soem screen heights. That's usually a sign that it should be pslit up into sub-functions. -- 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: 10 Gerrit-Owner: jolly <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: dexter <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-Attention: jolly <[email protected]> Gerrit-Comment-Date: Wed, 05 Jul 2023 13:36:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
