Attention is currently required from: laforge, neels, pespin. lynxis lazus 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: (7 comments) File src/libvlr/vlr.c: https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/af3cff48_9c583ff3?usp=... : PS5, Line 1490: return osmo_fsm_inst_dispatch(vsub->lu_fsm,

can go in same line. Done

File src/libvlr/vlr_lu_fsm.c: https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/3759abb5_720d31c8?usp=... : PS5, Line 357: *!

about the doxygen format comment: […] Done

https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/0fab90b5_12deb888?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" Done

https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/c79d807a_bbd82098?usp=... : PS5, Line 598: break;

(we often have a default: OSMO_ASSERT(false) in these to ensure we notice enum bugs) in timeouts?

https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/c3348b61_547ac77a?usp=... : PS5, Line 771: /*! count times timer T timed out */

. I would like to replace the words "count" and "out" with a word starting with t. Any ideas?

Not sure if this comment can be more cleared, because the spec depends on this knowledge anyways. https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/e6dfe964_6b4c48b9?usp=... : PS5, Line 1597: mian

typo Done

https://gerrit.osmocom.org/c/osmo-msc/+/38490/comment/85d503e9_20c3c779?usp=... : PS5, Line 1611: fi->T

technically, middle arg should be 0 or -1 or so, because vlr_is_cs() == false from above. […] In this case, you could also pass -1 to it. We could use `osmo_tdef_get(vlr_tdefs, ps_timer, OSMO_TDEF_S, 0);`, not sure if it makes it clearer the code instead of using vlr_timer_secs() as we did in all other places. vlr_timer_secs() is usually used by fsm_state_chg() and you pass the CS and PS timer values of the state into it, because CS has different Ts than PS.

-- 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 <lynxis@fe80.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann@sysmocom.de> Gerrit-Reviewer: laforge <laforge@osmocom.org> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> Gerrit-CC: pespin <pespin@sysmocom.de> Gerrit-Attention: neels <nhofmeyr@sysmocom.de> Gerrit-Attention: laforge <laforge@osmocom.org> Gerrit-Attention: pespin <pespin@sysmocom.de> Gerrit-Comment-Date: Tue, 28 Jan 2025 00:01:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr@sysmocom.de> Comment-In-Reply-To: pespin <pespin@sysmocom.de>