<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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">if the IMEI in the db were too short, we will feed -EINVAL into "%c". […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">With that being said, I could add a check like the following if you prefer:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br> if (*subscr->imei) {<br> char checksum = osmo_luhn(subscr->imei, 14);<br> if (checksum == -EINVAL)<br> vty_out(vty, " IMEI: %s (INVALID LENGTH!)%s", subscr->imei, VTY_NEWLINE);<br> else<br> vty_out(vty, " IMEI: %s%c%s", subscr->imei, checksum, VTY_NEWLINE);<br> }</pre></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"> vty_out(vty, "%% Checksum validated and stripped for search: imei = '%s'%s", id,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(I was going to complain about the checksum-digit-present-but-invalid case, but saw the vty test for […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Like that? ;)</p><p style="white-space: pre-wrap; word-wrap: break-word;">https://gerrit.osmocom.org/#/c/osmo-hlr/+/12527/8/src/hlr_vty_subscr.c@151</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 MSISDN (phone number)\n" \</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">here you say "14 digits" but above you explicitly allow 15. Maybe cut out the "(14 ... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Oh, that's a left over. Thanks, fixed!</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_UPDATE_HELP SUBSCR_HELP "Set or update subscriber data\n"</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">IIUC this string is used only once, so could drop the #define? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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"> imei = imei_buf;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">osmocom coding style is […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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">OsmoHLR# show subscriber imei 357613004448485</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">so, the user entered "...485" and is told "no ...48". […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Okay, it says now (right checksum, no entry in DB):</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> OsmoHLR# show subscriber imei 357613004448485<br> % Checksum validated and stripped for search: imei = '35761300444848'<br> % No subscriber for imei = '35761300444848'</pre><p style="white-space: pre-wrap; word-wrap: break-word;">and (wrong checksum):</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> OsmoHLR# show subscriber imei 357613004448484<br> % No subscriber for imei = '357613004448484'</pre><p style="white-space: pre-wrap; word-wrap: break-word;">and (right checksum, with entry in DB):</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> OsmoHLR# show subscriber imei 357613004448485<br> % Checksum validated and stripped for search: imei = '35761300444848'<br> ID: 1<br> IMSI: 123456789023000<br> MSISDN: none<br> IMEI: 357613004448485</pre></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"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(BTW, the 'subscriber foo show' ordering was a mistake by me. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">OK, changed to "show subscriber foo" ordering.</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"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">also have a valid checksum case plz […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></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: 11 </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: Tue, 22 Jan 2019 12:42:53 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>