Attention is currently required from: jolly. dexter 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: (5 comments) Patchset: PS10: I had a quick look. I only have a few cosmetic hints yet. File include/osmocom/msc/Makefile.am: https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/8fd36414_f95c638e PS10, Line 28: vgcs_fsm.h \ maybe rename this to "msc_vgcs" (not critical, but i see all the msc_ prefixes and msc_ho.h presumably has the FSM for handover) File include/osmocom/msc/vgcs_fsm.h: https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/69715b20_3dfd73fe PS10, Line 210: void _gsm44068_bcc_gcc_trans_free(struct gsm_trans *trans); why does this function begin with an underscore? To me this looks like a function that is not really public but needs to be called by a unittest, but I see you use this function in transaction.c File src/libmsc/vgcs_fsm.c: https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/141f40c0_335bdc3d PS10, Line 559: { VGCS_GCC_EV_TIMEOUT, "Timeout due to inactiviy" }, It probably makes more sense to stringify the constant names using OSMO_VALUE_STRING(). The reason for this is that it is then easier to grep for an event name in the code when it appears in the log. Just look at the other FSMs (e.g. handover_fsm.c from osmo-bsc) https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/66797acd_66cb0865 PS10, Line 951: #define connect_option false maybe use capital letters, so that it is immediately clear, that this is a define constant? -- 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: Tue, 04 Jul 2023 15:59:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
