osmith has posted comments on this change. ( https://gerrit.osmocom.org/12527 )
Change subject: VTY: integrate IMEI ...................................................................... Patch Set 11: (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". […] Is this necessary? This can only happen if the user modified the SQL db file directly without osmo-hlr. Because we are *always* checking the length, before writing into it (see db_hlr.c using osmo_imei_str_valid()). I don't think we are assuming that the user may change the database directly anywhere else. With that being said, I could add a check like the following if you prefer: if (*subscr->imei) { char checksum = osmo_luhn(subscr->imei, 14); if (checksum == -EINVAL) vty_out(vty, " IMEI: %s (INVALID LENGTH!)%s", subscr->imei, VTY_NEWLINE); else vty_out(vty, " IMEI: %s%c%s", subscr->imei, checksum, VTY_NEWLINE); } https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@150 PS10, Line 150: vty_out(vty, "%% Checksum validated and stripped for search: imei = '%s'%s", id, > (I was going to complain about the checksum-digit-present-but-invalid case, > but saw the vty test for […] Like that? ;) https://gerrit.osmocom.org/#/c/osmo-hlr/+/12527/8/src/hlr_vty_subscr.c@151 https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@167 PS10, Line 167: "Identify subscriber by MSISDN (phone number)\n" \ > here you say "14 digits" but above you explicitly allow 15. Maybe cut out the > "(14 ... […] Oh, that's a left over. Thanks, fixed! https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@176 PS10, Line 176: #define SUBSCR_UPDATE_HELP SUBSCR_HELP "Set or update subscriber data\n" > IIUC this string is used only once, so could drop the #define? […] Done https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@547 PS10, Line 547: imei = imei_buf; > osmocom coding style is […] Done 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: OsmoHLR# show subscriber imei 357613004448485 > so, the user entered "...485" and is told "no ...48". […] Okay, it says now (right checksum, no entry in DB): OsmoHLR# show subscriber imei 357613004448485 % Checksum validated and stripped for search: imei = '35761300444848' % No subscriber for imei = '35761300444848' and (wrong checksum): OsmoHLR# show subscriber imei 357613004448484 % No subscriber for imei = '357613004448484' and (right checksum, with entry in DB): OsmoHLR# show subscriber imei 357613004448485 % Checksum validated and stripped for search: imei = '35761300444848' ID: 1 IMSI: 123456789023000 MSISDN: none IMEI: 357613004448485 https://gerrit.osmocom.org/#/c/12527/10/tests/test_subscriber.vty@401 PS10, Line 401: > (BTW, the 'subscriber foo show' ordering was a mistake by me. […] OK, changed to "show subscriber foo" ordering. https://gerrit.osmocom.org/#/c/12527/10/tests/test_subscriber.vty@407 PS10, Line 407: > also have a valid checksum case plz […] Done -- 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: 11 Gerrit-Owner: osmith <osm...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Vadim Yanitskiy <axilira...@gmail.com> Gerrit-Reviewer: osmith <osm...@sysmocom.de> Gerrit-CC: Max <msur...@sysmocom.de> Gerrit-Comment-Date: Tue, 22 Jan 2019 12:42:53 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No