6 Jul
2023
6 Jul
'23
1:26 p.m.
Attention is currently required from: laforge, dexter.
jolly 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 11:
(19 comments)
File include/osmocom/msc/Makefile.am:
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/1821d973_f100ae98
PS10, Line 28: vgcs_fsm.h \
maybe rename this to "msc_vgcs" (not
critical, but i see all the msc_ prefixes and msc_ho. […]
Done
File include/osmocom/msc/vgcs_fsm.h:
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/77065a89_5ad25072
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 […]
This is the handler that is called
if a GCC/BCC transaction is freed. I used the same name style as for CC/SMS/SS transaction
types. You are right, it is confusing. I removed that dash.
File src/libmsc/vgcs_fsm.c:
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/9c7398f9_16be90bd
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 functi […]
Done
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/feb81000_468f9f21
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 o […]
Done
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/94bf49a5_99bc4520
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_ […]
Done
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/01118dc6_8583d3f8
PS10, Line 232: int _ie_invalid(void)
are those (_add_cause_ie, _add_callref_ie,
_msg_too_short, _ie_invalid) really intended to be expor […]
Done
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/3949c6df_7636e34f
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 […]
Done
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/f2629e0d_52b09a0f
PS10, Line 265: #if
not sure why this is commented out? because there are
no users yet and it's a static function? IF th […]
Done
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/3a067ee1_6842e9b2
PS10, Line 314: payload_len
as you're decrmenting this variable, I thing
I'd call it remaining_len or remaining_payload_len or s […]
Done
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/7edc85f0_b1b2935a
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 par […]
Done
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/0950e767_b4cee31a
PS10, Line 337: ie[0] +
here I think you're trusting ie[0] blindly,
without having done much checking on it? probably osmo_m […]
I forgot ti check the
return code of tlv_parse. Also I added check for each variable length IE fitting in the
message.
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/2dc8478c_86d879ef
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 co […]
I replaced this "4" by
the size of the IE payload. A variable "ie_len" holds the length and is then
used instead of repeating the integer.
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/d73c1367_9bb53d5c
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. […]
Done
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/182eb30a_de1d39c4
PS10, Line 416: if (ie[3] & 0x10) {
same comments as other decoders above.
If you
mean to check that the IE fits into the msg, its done.
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/91130837_00f2cbb0
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(). […]
Done
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/a0d25229_30d5f3c1
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 func […]
Done
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/cc354b62_31768561
PS10, Line 694: return -EINVAL;
here we return -EINVAL without talloc_free etc?
This will be done within the handling of the CLEAR event. I just added a comment
that states that.
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/95562024_6f95e65c
PS10, Line 951: #define connect_option false
maybe use capital letters, so that it is immediately
clear, that this is a define constant?
Done
https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/bf198b63_ffc797a1
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. […]
Done
--
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: 11
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: dexter <pmaier(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 06 Jul 2023 11:26:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: comment
720
days inactive
720
days old
0 comments
1 participants
participants (1)
-
jolly