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

[Neels Hofmeyr]

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 <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: Mon, 21 Jan 2019 17:28:00 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190121/a5070bf1/attachment.htm>