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