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 <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Comment-Date: Mon, 10 Jul 2023 18:31:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment