<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #d4ffd4;">Code-Review +1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/18942">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/18942/1/src/osmo-bsc/bsc_subscr_conn_fsm.c">File src/osmo-bsc/bsc_subscr_conn_fsm.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-bsc/+/18942/1/src/osmo-bsc/bsc_subscr_conn_fsm.c@794">Patch Set #1, Line 794:</a> <code style="font-family:monospace,monospace">           mi_imsi = data;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">When reading this code alone, i immediately ask myself:</p><ul><li>could data be NULL?</li><li>what if the mi->type is not an IMSI?</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">Of course when investigating, the event dispatch code does duly check that mi_imsi->type == GSM_MI_TYPE_IMSI, and always passes a data pointer.<br>Still would be nice to have an assert or if () for those assumptions here, too (or a comment)<br>so future readers are not compelled to grep for event dispatching code.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/18942/1/src/osmo-bsc/osmo_bsc_bssap.c">File src/osmo-bsc/osmo_bsc_bssap.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-bsc/+/18942/1/src/osmo-bsc/osmo_bsc_bssap.c@1078">Patch Set #1, Line 1078:</a> <code style="font-family:monospace,monospace">   } else if ((TLVP_VAL(&tp, GSM0808_IE_IMSI)[0] & GSM_MI_TYPE_MASK) != GSM_MI_TYPE_IMSI) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this is re-implementing part of osmo_mobile_identity_decode(), let's just drop this else-if because we're also doing the mi.type check below anyway...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/18942/1/src/osmo-bsc/osmo_bsc_bssap.c@1084">Patch Set #1, Line 1084:</a> <code style="font-family:monospace,monospace">mi_imsi.type != GSM_MI_TYPE_IMSI</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">...here</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/18942/1/src/osmo-bsc/osmo_bsc_bssap.c@1094">Patch Set #1, Line 1094:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(ws)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bsc/+/18942">change 18942</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-bsc/+/18942"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-bsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I52c43fb940f0db796adf4c0adb2260321c721c39 </div>
<div style="display:none"> Gerrit-Change-Number: 18942 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 22 Jun 2020 15:30:28 +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>