Change in osmo-hlr[master]: VTY: integrate IMEI

[osmith]

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 <osmith at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-CC: Max <msuraev at sysmocom.de>
Gerrit-Comment-Date: Tue, 22 Jan 2019 12:42:53 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190122/dbaeb5dc/attachment.htm>