Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12525 )
Change subject: Add IMEI column to subscriber table ...................................................................... Patch Set 4: (1 comment) -1 https://gerrit.osmocom.org/#/c/12525/4/src/db_hlr.c File src/db_hlr.c: https://gerrit.osmocom.org/#/c/12525/4/src/db_hlr.c@426 PS4, Line 426: LOGP(DAUC, LOGL_ERROR, "Update IMEI for subscriber ID='%s': SQL modified %d rows (expected 1)\n", imsi, > ID=<imsi> Argh hm, there's one thing I so far completely missed in this: would SQLite return 0 if the IMEI is already in the DB for this IMSI? Apologies, I might have sent us off on a tangent here. It appears to me that we can't distinguish between "no such IMSI" and "the IMEI was already exactly the given one". Can you briefly investigate whether we can get some SQL that has two different return codes for the two situations? If we can't, then we may after all still require a separate subscr_get_by_imsi followed by an IMEI update only if it differs, and my earlier preview generated some unnecessary work for you... :/ Or maybe the code path managing this already has the subscriber struct present somehow, maybe as part of the luop? Then we can check for differing IMEI there without another db hit ... and then we could also have used the ID as selector instead of the IMSI. Please add the scenario "setting the same IMEI again" to the unit test. Sorry again for missing that corner case before. -- To view, visit https://gerrit.osmocom.org/12525 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If232c80bea35d5c6864b889ae92d477eeaa3f45d Gerrit-Change-Number: 12525 Gerrit-PatchSet: 4 Gerrit-Owner: osmith <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: osmith <[email protected]> Gerrit-Comment-Date: Tue, 15 Jan 2019 15:57:47 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
