Patch Set 4: Code-Review+2 (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. Either of the two doesn't make sense. We can either ASSERT and remove the if, or we can kep he if (but then remove the *res from LOGPFSM outside of the if(res) clause. 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 unneccessarily long without any gain in information. We already know the type of VLR from the auto generated prefix, so the name "AUTH_FAILED" would be much better than "VLR_SUB_AS_AUTH_FAILED" in the log output. Not a critical issue for merging this, but I do have my reasons for almost never using OSMO_STRINGIFY. 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 know what is meant here. -- 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-HasComments: Yes
