Attention is currently required from: jolly. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/33611 )
Change subject: ASCI: Add processing and FSMs for VGCS/VBS ...................................................................... Patch Set 4: Code-Review-1 (4 comments) File src/osmo-bsc/vgcs_fsm.c: https://gerrit.osmocom.org/c/osmo-bsc/+/33611/comment/e407c8a1_38a80743 PS4, Line 72: sprintf(string, "%08u", callref); as I wrote elsewhere, I think it's good practice to use snprintf, even if (after review) we can assume 16 bytes size is large enough for 8 decimal digits plus NUL byte. https://gerrit.osmocom.org/c/osmo-bsc/+/33611/comment/5a31206b_0a3160c4 PS4, Line 451: vgcs_call_fsm I believe we had the discussion already at some other location: Please don't use the _fsm suffix in the name of the FSM. It is known from context that it is a FSM name. https://gerrit.osmocom.org/c/osmo-bsc/+/33611/comment/c8e10c9c_981cb59d PS4, Line 1044: vgcs_chan_fsm no _fsm suffix, please https://gerrit.osmocom.org/c/osmo-bsc/+/33611/comment/3dc05d57_525782cf PS4, Line 1093: LOGP(DASCI, LOGL_ERROR, "Unable to decode Channel Type.\n"); there are a lot of "raw" LOGP statements here, not providing any context. If you see this in the log, you will have no idea on whihc SCCP connection this was happening, related to which subscriber, which MSC, etc. only the first one has LOGPFSML() providing some context. Please always keep in mind that any real world use case will have more than one subscriber, so log messages must always carry some kind of context unless none is available at all. -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/33611 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id9e94fb4f27bb438b7093c031344a3400bfa34f1 Gerrit-Change-Number: 33611 Gerrit-PatchSet: 4 Gerrit-Owner: jolly <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Attention: jolly <[email protected]> Gerrit-Comment-Date: Mon, 10 Jul 2023 18:31:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
