<p>Patch set 2:<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-sgsn/+/15483">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15483/2//COMMIT_MSG">Commit Message:</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/15483/2//COMMIT_MSG@9">Patch Set #2, Line 9:</a> <code style="font-family:monospace,monospace">The user inactivity timer is similiar to the Gb READY timer and redruce</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">reduces</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15483/2/src/sgsn/gprs_mm_state_iu_fsm.c">File src/sgsn/gprs_mm_state_iu_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/15483/2/src/sgsn/gprs_mm_state_iu_fsm.c@15">Patch Set #2, Line 15:</a> <code style="font-family:monospace,monospace">        [ST_PMM_CONNECTED] = { .T=-3314 },</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So this timer is not defined in the case of Iu but we add it ourselves? please clarify that somewhere and leave a comment in code too if that's the case.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If timer comes from specs then it should be positive (and write a comment with the spec reference).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15483/2/src/sgsn/gprs_mm_state_iu_fsm.c@50">Patch Set #2, Line 50:</a> <code style="font-family:monospace,monospace">        static const struct RANAP_Cause user_inactive_cause = {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">why static? it's fine having it in the stack right?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15483/2/src/sgsn/gprs_mm_state_iu_fsm.c@96">Patch Set #2, Line 96:</a> <code style="font-family:monospace,monospace">               /* timer for pmm state. state=CONNECTED: -T3314 (User inactivity timer) */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ah here it is. Still, it's not clear if it comes from us or it's defined in specs. Better add the comment also in the timer struct definition at the start of the file.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15483/2/src/sgsn/sgsn_vty.c">File src/sgsn/sgsn_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/15483/2/src/sgsn/sgsn_vty.c@107">Patch Set #2, Line 107:</a> <code style="font-family:monospace,monospace">        /* non spec timers */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ah here it is! non-spec. Please add same thing in the FSM timer declaration.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-sgsn/+/15483">change 15483</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/+/15483"/><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: I66c2ac0350cb074aefd9a22c5121acf723f239d3 </div>
<div style="display:none"> Gerrit-Change-Number: 15483 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </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: laforge <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 12 Sep 2019 11:12:04 +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>