Patch Set 4: (3 comments)
https://gerrit.osmocom.org/#/c/3194/4/src/libvlr/vlr_access_req_fsm.c File src/libvlr/vlr_access_req_fsm.c: Line 416: if (res) { > we have an OSMO_ASSERT(res) above, and then a runtime if (res) here. Eithe agreed https://gerrit.osmocom.org/#/c/3194/4/src/libvlr/vlr_auth_fsm.c File src/libvlr/vlr_auth_fsm.c: Line 552: .name = OSMO_STRINGIFY(VLR_SUB_AS_AUTH_FAILED), > What I dislike about OSMO_STRINGIFY is that the log output gets unneccessar My reason to use it is that when I find a "made up" name like 'AUTH_FAILED' in a log, it is cumbersome to find the related FSM code. I really want to have the full state / event name in the log, so that I can directly search for it using ctags and grepping the files. It is often hard enough figure out how an FSM works, i.e. to sort out the relation of involved events and states. The combination of two things often made it impossibly hard for me to correlate the log output with what the code does: (1) having to keep in mind the mapping from "made up" names to event/state constants adds one step of indirection to each FSM detail, and (2) if two completely unrelated FSMs have similar or identical "made up" names, it is very easy to end up in the completely wrong area of code, often in the middle of almost figuring things out. I lost my train of thought a lot because of these issues when first reading the VLR code, hence my urgent desire for exact 1:1 names. I understand that from a perspective of administering a system, the shorter name may be easier to parse, but from a perspective of working with the code, the constants' complete names in the log make reading the situation infinitely easier. We could go for a dual approach, adding human readable names *as well as* the exact constants' names, and switch on and off printing the full constants depending on some debug flag? The LOGPFSM like macros could check for the log subsystem's logging level and include the full constant's name if it is below DEBUG -- but actually hard to do for separate logging targets with distinct logging levels. I guess it would rather be a compile time decision. Another idea would be to make the names of the constants more human readable where needed, so that we can keep the 1:1 relation. Would also be nice to pick up the constants' names without having to OSMO_STRINGIFY() them explicitly, but I guess that would take quite some macro magic. https://gerrit.osmocom.org/#/c/3194/4/src/libvlr/vlr_lu_fsm.c File src/libvlr/vlr_lu_fsm.c: Line 984: if (1) { // FIXME > a more verbose description of the FIXME would make sense. I don't really k Neither do I (IIRC it is code I took over from your initial patch). At first glance it seems that _start_lu_main() is sufficient, because as seen in the msc_vlr tests the VLR does send an IMEI id request when net->vlr->cfg.check_imei_rqd == true. However, no code path sends an IMEISV request. It seems this wants to make sure we always have an IMEISV, not only an IMEI. Also, I believe we want to set net->vlr->cfg.check_imei_rqd to true by default, not the case currently. ... possibly, make check_imei an enum to reflect {don't ask, ask for IMEI, ask for IMEISV}? ..... Ok, the investigation has now made me write the basis for a more concise FIXME text here, I suppose. -- To view, visit https://gerrit.osmocom.org/3194 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie303c98f8c18e40c87c1b68474b35de332033622 Gerrit-PatchSet: 4 Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-HasComments: Yes
