<p style="white-space: pre-wrap; word-wrap: break-word;">I'm advocating for OSMO_ASSERTS(); they would probably give us more SGSN crashes, but then we stand a chance of finding bugs related to a subscriber switching RAN types. The aim could also be to not crash and instead log errors and detach the subscriber, not so sure.</p><p><a href="https://gerrit.osmocom.org/c/osmo-sgsn/+/15186">View Change</a></p><p>7 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c">File src/gprs/gprs_gmm.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/15186/2/src/gprs/gprs_gmm.c@140">Patch Set #2, Line 140:</a> <code style="font-family:monospace,monospace">{</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Vadim's idea seems very good, and I would add:<br>Place an OSMO_ASSERT for matching RAN type to catch all bugs that mix and mess it up.<br>(Am not aware of the callers but would assume no mismatching RAN type state should ever be passed to this.)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@153">Patch Set #2, Line 153:</a> <code style="font-family:monospace,monospace"> case PMM_CONNECTED:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(while at it we could lose this 'case', or even make the entire switch into an if())</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@204">Patch Set #2, Line 204:</a> <code style="font-family:monospace,monospace"> mmctx_set_pmm_state(mm, PMM_IDLE);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">how could a RANAP_IU_EVENT come in on a ran_type != MM_CTX_T_UTRAN_Iu? I guess that should be an OSMO_ASSERT()?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@1096">Patch Set #2, Line 1096:</a> <code style="font-family:monospace,monospace"> case GSM48_MT_GMM_SERVICE_REQ:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">again this looks like it is clearly related to Iu and ran_type should always be UTRAN_Iu?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@2084">Patch Set #2, Line 2084:</a> <code style="font-family:monospace,monospace"> mmctx->gmm_state = GMM_REGISTERED_NORMAL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I know this was in the code before, but it seems to overwrite the previous state. Both RAN types set the state after this, so this is redundant and may confuse logging?<br>(It says "Changing state from X to Y", where the X is here always GMM_REGISTERED_NORMAL, although the state would have been something else before overwriting?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@2097">Patch Set #2, Line 2097:</a> <code style="font-family:monospace,monospace"> case MM_CTX_T_GERAN_Iu:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(instead of all of these dead switch cases, maybe we should rather "#if 0" away the unused enum member)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@2116">Patch Set #2, Line 2116:</a> <code style="font-family:monospace,monospace"> mmctx->gmm_state = GMM_REGISTERED_NORMAL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(again weird overwriting of the current state, like above)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-sgsn/+/15186">change 15186</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-sgsn/+/15186"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-sgsn </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I65ad9e180177bc9fc7c4a037cd85cfe33b161f73 </div>
<div style="display:none"> Gerrit-Change-Number: 15186 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-CC: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-CC: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 14 Aug 2019 23:39:43 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>