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
Attention is currently required from: jolly.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/33610 )
Change subject: ASCI: Add encoding of VGCS/VBS A-interface messages
......................................................................
Patch Set 3:
(1 comment)
File src/osmo-bsc/osmo_bsc_bssap.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/33610/comment/adc4e40f_70775948
PS3, Line 1749: void tx_setup_ack(struct gsm_subscriber_connection *conn, struct gsm0808_vgcs_feature_flags *ff)
all the other functions appear to be called bsc_tx_*
While there may not be any specific logic to have bsc_tx rather than tx_, I do think consistency within one source file is important.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/33610
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Ic2203a5119a226ac859a91dc42d81cbd71027ec8
Gerrit-Change-Number: 33610
Gerrit-PatchSet: 3
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Comment-Date: Mon, 10 Jul 2023 18:25:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment