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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: dexter <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: dexter <[email protected]> Gerrit-Comment-Date: Thu, 06 Jul 2023 11:26:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <[email protected]> Comment-In-Reply-To: dexter <[email protected]> Gerrit-MessageType: comment
