Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12527 )
Change subject: VTY: integrate IMEI ...................................................................... Patch Set 10: Code-Review-1 (8 comments) https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c File src/hlr_vty_subscr.c: https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@62 PS10, Line 62: vty_out(vty, " IMEI: %s%c%s", subscr->imei, osmo_luhn(subscr->imei, 14), VTY_NEWLINE); if the IMEI in the db were too short, we will feed -EINVAL into "%c". Please make sure to avoid that. https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@150 PS10, Line 150: } (I was going to complain about the checksum-digit-present-but-invalid case, but saw the vty test for this answering with "does not exist", which is fine.) https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@167 PS10, Line 167: "Identify subscriber by IMEI (14 digits, without 15th check digit)\n" \ here you say "14 digits" but above you explicitly allow 15. Maybe cut out the "(14 ...)"? https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@176 PS10, Line 176: #define SUBSCR_IMEI_HELP "Set IMEI of the subscriber (debug only! normally set from MSC, do not use manually)\n" IIUC this string is used only once, so could drop the #define? (to put this review in context: apparently the MSISDN one should also have been dropped. not in this patch.) Maybe weaken the warning a bit: "(Normally, this is populated from the MSC, no need to set this manually.)", also because the 'none' below to clear an IMEI could be a useful use case not covered by the MSC. https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@547 PS10, Line 547: else if (!osmo_imei_str_valid(imei, false)) { osmocom coding style is } else if () { (wouldn't be my choice either, but that's how it is.) https://gerrit.osmocom.org/#/c/12527/10/tests/test_subscriber.vty File tests/test_subscriber.vty: https://gerrit.osmocom.org/#/c/12527/10/tests/test_subscriber.vty@50 PS10, Line 50: % No subscriber for imei = '35761300444848' so, the user entered "...485" and is told "no ...48". If I were the user I would file a bug report. This is a bit hard to solve, right? Maybe the vty could simply indicate that it is using the shorter IMEI version? OsmoHLR# show subscriber imei 357613004448485 % Stripping checksum digit: imei = '35761300444848' % No subscriber for imei = '35761300444848' or something, idk https://gerrit.osmocom.org/#/c/12527/10/tests/test_subscriber.vty@401 PS10, Line 401: OsmoHLR# subscriber imei 35761300444848 show (BTW, the 'subscriber foo show' ordering was a mistake by me. All other vty commands everywhere use the order of 'show subscriber foo'.) https://gerrit.osmocom.org/#/c/12527/10/tests/test_subscriber.vty@407 PS10, Line 407: % No subscriber for imei = '357613004448484' also have a valid checksum case plz # show subscriber imei 357613004448485 -- To view, visit https://gerrit.osmocom.org/12527 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: I1af7b573ca2a1cb22497052665012d9c1acf3b30 Gerrit-Change-Number: 12527 Gerrit-PatchSet: 10 Gerrit-Owner: osmith <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Vadim Yanitskiy <[email protected]> Gerrit-Reviewer: osmith <[email protected]> Gerrit-CC: Max <[email protected]> Gerrit-Comment-Date: Mon, 21 Jan 2019 17:28:00 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
