<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/9211">View Change</a></p><p>13 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9211/2/include/osmocom/msc/gsm_subscriber.h">File include/osmocom/msc/gsm_subscriber.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/9211/2/include/osmocom/msc/gsm_subscriber.h@20">Patch Set #2, Line 20:</a> <code style="font-family:monospace,monospace">#define GSM_SUBSCRIBER_NO_EXPIRATION    0x0</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">interesting that it existed before, but I would prefer these values moving into the libvlr code now. After all it is specifically about VLR expiry. And then I'd call them VLR_SUBSRC_*, not GSM_*</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c">File src/libvlr/vlr.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/9211/2/src/libvlr/vlr.c@469">Patch Set #2, Line 469:</a> <code style="font-family:monospace,monospace">   /* Table 10.5.33: The T3212 timeout value is coded as the</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(why not use the entire line width. hint in vim: Visual-Line mark the comment and enter gw to automatically re-align the line widths to the value set in 'set textwidth=N'. With 'set cindent' vim even edits the '*' markers for you automatically.)</p><p style="white-space: pre-wrap; word-wrap: break-word;">The "Table X" info is pretty useless without the "3GPP TS XX.YYY" spec.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@471">Patch Set #2, Line 471:</a> <code style="font-family:monospace,monospace">        * periodic updating in decihours. Mark the subscriber as</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"decihours" lol</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@473">Patch Set #2, Line 473:</a> <code style="font-family:monospace,monospace">  * Timeout is twice the t3212 value plus one minute */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">lol "twice plus one minute" wtf</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@479">Patch Set #2, Line 479:</a> <code style="font-family:monospace,monospace">                 vsub->imsi, vsub->id);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">use vlr_subscr_name() (see below)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@481">Patch Set #2, Line 481:</a> <code style="font-family:monospace,monospace">         vsub->expire_lu = GSM_SUBSCRIBER_NO_EXPIRATION;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">hmm. we should always be getting the time, right? If not, then what do we prefer, "memory leaks" of subscribers sticking around, or a subscriber being thrown out early and needing to do another LU later? I actually think rather the latter. So I think I'd set expire_lu = 1 so it always gets expired?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@503">Patch Set #2, Line 503:</a> <code style="font-family:monospace,monospace">                LOGP(DVLR, LOGL_DEBUG, "IMSI=%s id=%llu: Location Update expired\n", vsub->imsi, vsub->id);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">use vlr_subscr_name(). We use that all over, and we might at some point tweak its output, which would then match everywhere, including here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@1192">Patch Set #2, Line 1192:</a> <code style="font-family:monospace,monospace">  osmo_timer_setup(&vlr->lu_expire_timer, vlr_subscr_expire_lu, vlr);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(for me it would make sense to keep osmo_timer_setup() right next to osmo_timer_schedule() below)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr_lu_fsm.c">File src/libvlr/vlr_lu_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/9211/2/src/libvlr/vlr_lu_fsm.c@359">Patch Set #2, Line 359:</a> <code style="font-family:monospace,monospace">            /* Balanced by vlr_subscr_rx_imsi_detach() or Location Update expiry */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">let's rather say "by vlr_subscr_expire()", which is accurate for both cases.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9211/2/tests/msc_vlr/msc_vlr_test_no_authen.c">File tests/msc_vlr/msc_vlr_test_no_authen.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/9211/2/tests/msc_vlr/msc_vlr_test_no_authen.c@929">Patch Set #2, Line 929:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">maybe you should set an explicit t3212 here to make the test independent from default values possibly changing one day (meaning different timer values ending up in the checked log output)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9211/2/tests/msc_vlr/msc_vlr_test_no_authen.c@971">Patch Set #2, Line 971:</a> <code style="font-family:monospace,monospace">   OSMO_ASSERT(vsub);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">also assert that lu_success flag and the periodic timer value being != that GSM_SUBSCRIBER_NO_EXPIRATION value?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9211/2/tests/msc_vlr/msc_vlr_test_no_authen.c@973">Patch Set #2, Line 973:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would add a fake_time_passes() here of just under the expiration time and assert that the subscriber is still around, and then pass that last minute of fake time and assert it is gone. (somehow make sure the periodic lu cleanup timer has also fired, probably by enough fake time passing, as you apparently do).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Note that fake_time is quite precise since it is not subject to real time passing, and you should be able to stay even a millisecond below / a millisecond above to not trigger / trigger the timer (as long as the periodic cleanup function gets invoked); however, i've never used fake time with MONOTONIC before, I hope that works?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9211/2/tests/msc_vlr/msc_vlr_tests.h">File tests/msc_vlr/msc_vlr_tests.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/9211/2/tests/msc_vlr/msc_vlr_tests.h@221">Patch Set #2, Line 221:</a> <code style="font-family:monospace,monospace"> osmo_clock_override_add(CLOCK_MONOTONIC, secs, usecs * 1000); \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ah. nice.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/9211">change 9211</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/9211"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-msc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9 </div>
<div style="display:none"> Gerrit-Change-Number: 9211 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Stefan Sperling <ssperling@sysmocom.de> </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: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Stefan Sperling <ssperling@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 17 May 2018 16:25:06 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>