Patch Set 6: (3 comments)
all in all I think this patch can stay as it is, but I need to fix the db API docs: https://gerrit.osmocom.org/#/c/4311/6/src/ctrl.c File src/ctrl.c: Line 233: aud3g.algo = OSMO_AUTH_ALG_NONE; > I'm confused by this part. In case of ENOENT error we will continue with pr ah, I think that might have come from the VTY code where I used to print "2G auth data: none" in some earlier state of the patches, so that I wanted to output absence and still want to call the printing functions below; doesn't apply here, that's right. Returning immediately is however just a minor optimization, really. In general, ENOENT indicates that no auth data is present, which is a valid situation. Line 258: rc = db_get_auth_data(hlr->dbc, subscr.imsi, &aud2g, &aud3g, NULL); > If someone later have to change db-get_auth_data() they got to make sure no ENOENT rc is a concept used throughout the db API of osmo-hlr, it is imperative to not break that in case of changes. It is documented for all the db functions. ...at least I thought so, I see now that db_get_auth_data() was changed to the ENOENT rc but I forgot to adjust the API comment. Other functions also need fixing of their API doc, it should all read something like db_subscr_nam()'s doc: * Returns 0 on success, -ENOENT when the given IMSI does not exist, -EINVAL if * the SQL statement could not be composed, -ENOEXEC if running the SQL * statement failed, -EIO if the amount of rows modified is unexpected. Line 262: aud3g.algo = OSMO_AUTH_ALG_NONE; > Same here. Maybe move it to separate inline function? here it makes sense to continue. We gather data, then print it below. Note the print_subscr_info() that we still want to call even if no auth data is present. So I would keep this as is, so that ENOENT makes sure to mark auth data as not present and then go on to print whatever we got. I think the two instances of rc evaluation don't justify effort to undup the code yet... but it would make sense in principle, agreed. -- To view, visit https://gerrit.osmocom.org/4311 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50 Gerrit-PatchSet: 6 Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-HasComments: Yes
