<p>Patch set 10:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/12527">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c">File src/hlr_vty_subscr.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@62">Patch Set #10, Line 62:</a> <code style="font-family:monospace,monospace"> vty_out(vty, " IMEI: %s%c%s", subscr->imei, osmo_luhn(subscr->imei, 14), VTY_NEWLINE);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">if the IMEI in the db were too short, we will feed -EINVAL into "%c". Please make sure to avoid that.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@150">Patch Set #10, Line 150:</a> <code style="font-family:monospace,monospace"> }</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(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.)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@167">Patch Set #10, Line 167:</a> <code style="font-family:monospace,monospace"> "Identify subscriber by IMEI (14 digits, without 15th check digit)\n" \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">here you say "14 digits" but above you explicitly allow 15. Maybe cut out the "(14 ...)"?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@176">Patch Set #10, Line 176:</a> <code style="font-family:monospace,monospace">#define SUBSCR_IMEI_HELP "Set IMEI of the subscriber (debug only! normally set from MSC, do not use manually)\n"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">IIUC this string is used only once, so could drop the #define?</p><p style="white-space: pre-wrap; word-wrap: break-word;">(to put this review in context: apparently the MSISDN one should also have been dropped. not in this patch.)</p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe weaken the warning a bit: "(Normally, this is populated from the MSC, no need to set this manually.)",<br>also because the 'none' below to clear an IMEI could be a useful use case not covered by the MSC.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12527/10/src/hlr_vty_subscr.c@547">Patch Set #10, Line 547:</a> <code style="font-family:monospace,monospace"> else if (!osmo_imei_str_valid(imei, false)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">osmocom coding style is</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> } else if () {</pre><p style="white-space: pre-wrap; word-wrap: break-word;">(wouldn't be my choice either, but that's how it is.)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12527/10/tests/test_subscriber.vty">File tests/test_subscriber.vty:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12527/10/tests/test_subscriber.vty@50">Patch Set #10, Line 50:</a> <code style="font-family:monospace,monospace">% No subscriber for imei = '35761300444848'</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">so, the user entered "...485" and is told "no ...48".<br>If I were the user I would file a bug report.<br>This is a bit hard to solve, right? Maybe the vty could simply indicate that it is using the shorter IMEI version?</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> OsmoHLR# show subscriber imei 357613004448485<br> % Stripping checksum digit: imei = '35761300444848'<br> % No subscriber for imei = '35761300444848'</pre><p style="white-space: pre-wrap; word-wrap: break-word;">or something, idk</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12527/10/tests/test_subscriber.vty@401">Patch Set #10, Line 401:</a> <code style="font-family:monospace,monospace">OsmoHLR# subscriber imei 35761300444848 show</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(BTW, the 'subscriber foo show' ordering was a mistake by me. All other vty commands everywhere use the order of 'show subscriber foo'.)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12527/10/tests/test_subscriber.vty@407">Patch Set #10, Line 407:</a> <code style="font-family:monospace,monospace">% No subscriber for imei = '357613004448484'</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">also have a valid checksum case plz</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> # show subscriber imei 357613004448485</pre></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12527">change 12527</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/12527"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-hlr </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I1af7b573ca2a1cb22497052665012d9c1acf3b30 </div>
<div style="display:none"> Gerrit-Change-Number: 12527 </div>
<div style="display:none"> Gerrit-PatchSet: 10 </div>
<div style="display:none"> Gerrit-Owner: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 21 Jan 2019 17:28:00 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>