<p>Patch set 1:<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/9257">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.h">File src/gprs/gprs_gmm_attach.h:</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/9257/1/src/gprs/gprs_gmm_attach.h@7">Patch Set #1, Line 7:</a> <code style="font-family:monospace,monospace"> ST_INIT,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">the states might warrant some more documentation/explanation in comments here</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.h@17">Patch Set #1, Line 17:</a> <code style="font-family:monospace,monospace">      E_ATTACH_REQ_RECV,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same goes for the events here.  The *_RECV and *_SENT are pretty lcear, but the others might not be?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c">File src/gprs/gprs_gmm_attach.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/9257/1/src/gprs/gprs_gmm_attach.c@10">Patch Set #1, Line 10:</a> <code style="font-family:monospace,monospace">extern const struct value_string gmm_attach_req_fsm_event_names[];</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">why declare the fsm and the event_names here?  Are they used anywhere in the filbe before they are defined further down?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c@71">Patch Set #1, Line 71:</a> <code style="font-family:monospace,monospace">   /* check if we received a identity response */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">you don't have a switch statement or an OSMO_ASSERT on the "event" which mgiht be dangerous as the FSM definition might be edited in the future and this code implicitly assumes only a single event may arrive here?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c@89">Patch Set #1, Line 89:</a> <code style="font-family:monospace,monospace">  if (type == GSM_MI_TYPE_IMEI && !strlen(ctx->imsi)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">no tab here</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c@143">Patch Set #1, Line 143:</a> <code style="font-family:monospace,monospace">              /* FIXME!! */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this appears that UMTS AKA is not working with this new FSM?  Was it also broken before this FSM?  In that case: We cannot afford to introduce known regression.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c@155">Patch Set #1, Line 155:</a> <code style="font-family:monospace,monospace">    }</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">do we want to silently ignore any other events?  In other FSMs we explicitly OSMO_ASSERT() on any unexpecte events here.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Same applies to all other state handling functiosn.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c@175">Patch Set #1, Line 175:</a> <code style="font-family:monospace,monospace">          /* TODO: #ifdef ! PTMSI_ALLOC is not supported */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">does this mean you're not supporting disabling or enabling of P-TMSI allocation?  I believe it should be enabled at all times these days, the #define was just a hack to disable it for debugging?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c@222">Patch Set #1, Line 222:</a> <code style="font-family:monospace,monospace">          .name = "Ask the hlr about the MS",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">please use shorter more symbolic names, possibly even "ST_ASK_VLR" here.  This is used heavily in logging and also in the CTRL interface, not sure spaces are even permitted...</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/9257">change 9257</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/9257"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I58b9c17be9776a03bb2a5b21e99135cfefc8c912 </div>
<div style="display:none"> Gerrit-Change-Number: 9257 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 23 May 2018 16:19:48 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>