Change in osmo-msc[master]: implement periodic Location Update expiry in the VLR

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Thu May 17 16:25:06 UTC 2018


Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/9211 )

Change subject: implement periodic Location Update expiry in the VLR
......................................................................


Patch Set 2: Code-Review-1

(13 comments)

https://gerrit.osmocom.org/#/c/9211/2/include/osmocom/msc/gsm_subscriber.h
File include/osmocom/msc/gsm_subscriber.h:

https://gerrit.osmocom.org/#/c/9211/2/include/osmocom/msc/gsm_subscriber.h@20
PS2, Line 20: #define GSM_SUBSCRIBER_NO_EXPIRATION	0x0
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_*


https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c
File src/libvlr/vlr.c:

https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@469
PS2, Line 469: 	/* Table 10.5.33: The T3212 timeout value is coded as the
(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.)

The "Table X" info is pretty useless without the "3GPP TS XX.YYY" spec.


https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@471
PS2, Line 471: 	 * periodic updating in decihours. Mark the subscriber as
"decihours" lol


https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@473
PS2, Line 473: 	 * Timeout is twice the t3212 value plus one minute */
lol "twice plus one minute" wtf


https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@479
PS2, Line 479: 		     vsub->imsi, vsub->id);
use vlr_subscr_name() (see below)


https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@481
PS2, Line 481: 		vsub->expire_lu = GSM_SUBSCRIBER_NO_EXPIRATION;
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?


https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@503
PS2, Line 503: 		LOGP(DVLR, LOGL_DEBUG, "IMSI=%s id=%llu: Location Update expired\n", vsub->imsi, vsub->id);
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.


https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@1192
PS2, Line 1192: 	osmo_timer_setup(&vlr->lu_expire_timer, vlr_subscr_expire_lu, vlr);
(for me it would make sense to keep osmo_timer_setup() right next to osmo_timer_schedule() below)


https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr_lu_fsm.c
File src/libvlr/vlr_lu_fsm.c:

https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr_lu_fsm.c@359
PS2, Line 359: 		/* Balanced by vlr_subscr_rx_imsi_detach() or Location Update expiry */
let's rather say "by vlr_subscr_expire()", which is accurate for both cases.


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:

https://gerrit.osmocom.org/#/c/9211/2/tests/msc_vlr/msc_vlr_test_no_authen.c@929
PS2, Line 929: 
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)


https://gerrit.osmocom.org/#/c/9211/2/tests/msc_vlr/msc_vlr_test_no_authen.c@971
PS2, Line 971: 	OSMO_ASSERT(vsub);
also assert that lu_success flag and the periodic timer value being != that GSM_SUBSCRIBER_NO_EXPIRATION value?


https://gerrit.osmocom.org/#/c/9211/2/tests/msc_vlr/msc_vlr_test_no_authen.c@973
PS2, Line 973: 
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).

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?


https://gerrit.osmocom.org/#/c/9211/2/tests/msc_vlr/msc_vlr_tests.h
File tests/msc_vlr/msc_vlr_tests.h:

https://gerrit.osmocom.org/#/c/9211/2/tests/msc_vlr/msc_vlr_tests.h@221
PS2, Line 221: 	osmo_clock_override_add(CLOCK_MONOTONIC, secs, usecs * 1000); \
ah. nice.



-- 
To view, visit https://gerrit.osmocom.org/9211
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9
Gerrit-Change-Number: 9211
Gerrit-PatchSet: 2
Gerrit-Owner: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-Comment-Date: Thu, 17 May 2018 16:25:06 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180517/22e859f8/attachment.htm>


More information about the gerrit-log mailing list