Attention is currently required from: laforge, lynxis lazus, pespin. neels has posted comments on this change by lynxis lazus. ( https://gerrit.osmocom.org/c/osmo-msc/+/38490?usp=email ) Change subject: vlr: add PS support ...................................................................... Patch Set 5: Code-Review+1 (14 comments) Patchset: PS5: just some cosmetic and maintainability comments. File src/libvlr/vlr_lu_fsm.c: https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/7abdc8f4_c58932b8?usp… : PS5, Line 357: *! about the doxygen format comment: the `/*!` is in the wrong place here. To be associated with the 'N' member, a `/*! ... */` doxygen comment needs to go above the item. For inline commenting, you need to use `/*< ... */` instead. (no need to use doxygen IMHO, and IMHO comments above are in general nicer than inline ones, less clutter in future editing.) https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/9963069f_44d3e2b1?usp… : PS5, Line 569: static void vlr_lu_compl_fsm_reset_n(struct osmo_fsm_inst *fi, uint32_t prev_state) I think this does not need to change -- this function is called for multiple different FSM onenter events, so there is no single accurate name. So I think the naming is actually chosen well. We could have three individually named my_state_name_onenter() functions and each of them calls the non-duplicated vlr_lu_compl_fsm_reset_n() function. That would be ultra perfect but given the short function body, this is clearly clutter, which is also a thing to aviod. All in all this is an even steven, on a purely cosmetic item, and i would just leave the patch as it is. https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/08254f62_88407a50?usp… : PS5, Line 587: return 1; (personally i prefer incrementing on a separate code line, for more trivial code clarity) https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/1b4bc96b_414fde38?usp… : PS5, Line 589: LOGPFSML(fi, LOGL_ERROR, "LU Compl timeout %d / %d\n", fi->T, lcvp->N); let's have "T%d / N%d" https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/3217ef09_af7c2e47?usp… : PS5, Line 598: break; (we often have a default: OSMO_ASSERT(false) in these to ensure we notice enum bugs) https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/fb26f96e_e605c48a?usp… : PS5, Line 771: /*! count times timer T timed out */ (maybe: "count nr of times that timer...") https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/e19badc2_afbf9456?usp… : PS5, Line 1437: static void lu_fsm_reset_n(struct osmo_fsm_inst *fi, uint32_t prev_state) i agree with pau here because it is only one FSM state's onenter action https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/2f512e72_c77a3092?usp… : PS5, Line 1570: { (would be nice to have a short one-liner comment to indicate what this does / why it needs to go in the rather unusual _preterm()) https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/4331c108_c82d930f?usp… : PS5, Line 1582: RVIV a libosmocore typo? https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/66157004_af050256?usp… : PS5, Line 1586: gsm48_cause = GSM48_REJECT_NETWORK_FAILURE; (add "break;" in the end) https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/22e5df51_1de64d5a?usp… : PS5, Line 1597: mian typo https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/364b8e8e_e12b4583?usp… : PS5, Line 1611: fi->T technically, middle arg should be 0 or -1 or so, because vlr_is_cs() == false from above. (btw, i wonder a bit why it is necessary to pass two args to vlr_timer_secs(), still waiting for the caller that passes two distinct values. the patch adding the second arg is already merged.) https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/dc8d3532_034439a0?usp… : PS5, Line 1742: (two blank lines, unusual for osmocom) -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/38490?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: Ie9ffeb140c9d354b3a0f4822e2619f623235add0 Gerrit-Change-Number: 38490 Gerrit-PatchSet: 5 Gerrit-Owner: lynxis lazus &lt;lynxis(a)fe80.eu&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel &lt;dwillmann(a)sysmocom.de&gt; Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Reviewer: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-CC: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: lynxis lazus &lt;lynxis(a)fe80.eu&gt; Gerrit-Comment-Date: Wed, 15 Jan 2025 14:31:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt; Comment-In-Reply-To: lynxis lazus &lt;lynxis(a)fe80.eu&gt;