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'll copy the same code in here then. […]
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)
Acknowledged
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)