<p>Patch set 4:<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/c/osmo-hlr/+/16808">View Change</a></p><p>6 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16808/4/src/hlr_vty.c">File src/hlr_vty.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/osmo-hlr/+/16808/4/src/hlr_vty.c@49">Patch Set #4, Line 49:</a> <code style="font-family:monospace,monospace">"roaming-not-allowed" },</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">No need to break the line.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16808/4/src/hlr_vty.c@85">Patch Set #4, Line 85:</a> <code style="font-family:monospace,monospace">get_value_string_or_null</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We should not get NULL if everything is correct I think.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16808/4/src/hlr_vty.c@88">Patch Set #4, Line 88:</a> <code style="font-family:monospace,monospace">imsi_cause_code</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why not just g_hlr->imsi_unknown_cause != GMM_CAUSE_IMSI_UNKNOWN (it's the default).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16808/4/src/hlr_vty.c@356">Patch Set #4, Line 356:</a> <code style="font-family:monospace,monospace">imsi unknown cause</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You have to add help string for every singe word of the command. Otherwise help message would be broken. Feel free to replace three vectors "imsi unknown cause" by one "subscriber-reject-cause" (so we can also avoid repeating "imsi unknown cause imsi-unknown"):</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  "subscriber-reject-cause (...)",<br>  "Define Location Updating Reject cause to be send in case the subscriber is not known\n"<br>  "Cause value 0xXX (IMSI unknown)\n"<br>  "Cause value 0xXX (Roaming not allowed)"</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16808/4/src/hlr_vty.c@357">Patch Set #4, Line 357:</a> <code style="font-family:monospace,monospace">GSUP cause</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is actually GSM 04.08 GMM cause if I understand correctly.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16808/4/src/hlr_vty.c@365">Patch Set #4, Line 365:</a> <code style="font-family:monospace,monospace">Cannot find cause value to %s%s</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This shall not happen because VTY would not accept any other values than 'imsi-unknown' and 'roaming-not-allowed'. You can just do OSMO_ASSERT() to avoid Coverity warnings.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16808">change 16808</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/c/osmo-hlr/+/16808"/><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-Change-Id: Icea39020c23fbbea9e92847df76af8986fdbf48a </div>
<div style="display:none"> Gerrit-Change-Number: 16808 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 23 Jan 2020 23:57:10 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>