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

[Stefan Sperling]

Stefan Sperling has submitted this change and it was merged. ( 
https://gerrit.osmocom.org/9211 )

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

implement periodic Location Update expiry in the VLR

Remove subscribers which fail to send periodic Location Updates from the
list of subscribers known to the VLR. This complements the IMSI detach
procedure: periodic LU expiry triggers an implicit IMSI detach.

Expired subscribers are purged from a periodic timer which iterates
over all subscribers once per minute.

Subscribers with an active connection do not expire. This is controlled
by the subscriber conn FSM which sets a subscriber's the LU expiry timeout
value to GSM_SUBSCRIBER_NO_EXPIRATION while a connection is active.

Add support for fake time with osmo_clock_gettime() to msc_vlr tests.

This functionality existed in OpenBSC but was lost during the nitb split.
This code took some inspiration from the OpenBSC implementation.

Related: OS#1976
Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9
---
M include/osmocom/msc/gsm_subscriber.h
M include/osmocom/msc/vlr.h
M src/libmsc/subscr_conn.c
M src/libvlr/vlr.c
M src/libvlr/vlr_lu_fsm.c
M tests/msc_vlr/msc_vlr_test_no_authen.c
M tests/msc_vlr/msc_vlr_test_no_authen.err
M tests/msc_vlr/msc_vlr_tests.c
M tests/msc_vlr/msc_vlr_tests.h
9 files changed, 264 insertions(+), 3 deletions(-)

Approvals:
  Neels Hofmeyr: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/include/osmocom/msc/gsm_subscriber.h 
b/include/osmocom/msc/gsm_subscriber.h
index 16e1037..01d9c58 100644
--- a/include/osmocom/msc/gsm_subscriber.h
+++ b/include/osmocom/msc/gsm_subscriber.h
@@ -17,8 +17,6 @@
 #define GSM_SUBSCRIBER_FIRST_CONTACT   0x0001
 /* gprs_sgsn.h defines additional flags including and above bit 16 (0x1) */

-#define GSM_SUBSCRIBER_NO_EXPIRATION   0x0
-
 enum gsm_subscriber_field {
GSM_SUBSCRIBER_IMSI,
GSM_SUBSCRIBER_TMSI,
diff --git a/include/osmocom/msc/vlr.h b/include/osmocom/msc/vlr.h
index 0a9ef6f..f12d758 100644
--- a/include/osmocom/msc/vlr.h
+++ b/include/osmocom/msc/vlr.h
@@ -20,6 +20,9 @@

 struct log_target;

+#define VLR_SUBSCRIBER_NO_EXPIRATION   0
+#define VLR_SUBSCRIBER_LU_EXPIRATION_INTERVAL  60  /* in seconds */
+
 /* from 3s to 10s */
 #define GSM_29002_TIMER_S  10
 /* from 15s to 30s */
@@ -148,6 +151,7 @@
struct osmo_fsm_inst *proc_arq_fsm;

bool lu_complete;
+   time_t expire_lu;

void *msc_conn_ref;

@@ -237,6 +241,7 @@
struct llist_head operations;
struct gsup_client *gsup_client;
struct vlr_ops ops;
+   struct osmo_timer_list lu_expire_timer;
struct {
bool retrieve_imeisv_early;
bool retrieve_imeisv_ciphered;
@@ -379,6 +384,7 @@
  enum osmo_fsm_term_cause fsm_cause,
  uint8_t gsm48_cause);

+void vlr_subscr_enable_expire_lu(struct vlr_subscr *vsub);

 /* Process Acccess Request FSM */
 
diff --git a/src/libmsc/subscr_conn.c b/src/libmsc/subscr_conn.c
index 1b3b240..c1d0e11 100644
--- a/src/libmsc/subscr_conn.c
+++ b/src/libmsc/subscr_conn.c
@@ -202,6 +202,15 @@

 static void subscr_conn_fsm_accepted_enter(struct osmo_fsm_inst *fi, uint32_t 
prev_state)
 {
+   struct gsm_subscriber_connection *conn = fi->priv;
+
+   /* Stop Location Update expiry for this subscriber. While the subscriber
+* has an open connection the LU expiry timer must remain disabled.
+* Otherwise we would kick the subscriber off the network when the timer
+* expires e.g. during a long phone call.
+* The LU expiry timer will restart once the connection is closed. */
+   conn->vsub->expire_lu = VLR_SUBSCRIBER_NO_EXPIRATION;
+
if (!subscr_conn_fsm_has_active_transactions(fi))
osmo_fsm_inst_dispatch(fi, SUBSCR_CONN_E_UNUSED, NULL);
 }
@@ -278,6 +287,12 @@
/* Cancel all VLR FSMs, if any */
vlr_subscr_cancel_attach_fsm(conn->vsub, OSMO_FSM_TERM_ERROR, 
GSM48_REJECT_CONGESTION);

+   if (conn->vsub) {
+   /* The subscriber has no active connection anymore.
+* Restart the periodic Location Update expiry timer for this 
subscriber. */
+   vlr_subscr_enable_expire_lu(conn->vsub);
+   }
+
/* If we're closing in a middle of a trans, we need to clean up */
trans_conn_closed(conn);

diff --git a/src/libvlr/vlr.c b/src/libvlr/vlr.c
index 2d232be..29098b1 100644
--- a/src/libvlr/vlr.c
+++ b/src/libvlr/vlr.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -460,6 +461,50 @@
return 0;
 }

+void vlr_subscr_enable_expire_lu(struct vlr_subscr *vsub)
+{
+   struct gsm_network *net = vsub->vlr->user_ctx; /* XXX move t3212 into 
struct vlr_instance? */
+   struct timespec now;
+
+   /* The 

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

[Neels Hofmeyr]

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 7: Code-Review+2


--
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: 7
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Stefan Sperling 
Gerrit-Comment-Date: Wed, 23 May 2018 12:55:59 +
Gerrit-HasComments: No
Gerrit-HasLabels: Yes

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

[Neels Hofmeyr]

Neels Hofmeyr has uploaded a new patch set (#7) to the change originally 
created by Stefan Sperling. ( https://gerrit.osmocom.org/9211 )

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

implement periodic Location Update expiry in the VLR

Remove subscribers which fail to send periodic Location Updates from the
list of subscribers known to the VLR. This complements the IMSI detach
procedure: periodic LU expiry triggers an implicit IMSI detach.

Expired subscribers are purged from a periodic timer which iterates
over all subscribers once per minute.

Subscribers with an active connection do not expire. This is controlled
by the subscriber conn FSM which sets a subscriber's the LU expiry timeout
value to GSM_SUBSCRIBER_NO_EXPIRATION while a connection is active.

Add support for fake time with osmo_clock_gettime() to msc_vlr tests.

This functionality existed in OpenBSC but was lost during the nitb split.
This code took some inspiration from the OpenBSC implementation.

Related: OS#1976
Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9
---
M include/osmocom/msc/gsm_subscriber.h
M include/osmocom/msc/vlr.h
M src/libmsc/subscr_conn.c
M src/libvlr/vlr.c
M src/libvlr/vlr_lu_fsm.c
M tests/msc_vlr/msc_vlr_test_no_authen.c
M tests/msc_vlr/msc_vlr_test_no_authen.err
M tests/msc_vlr/msc_vlr_tests.c
M tests/msc_vlr/msc_vlr_tests.h
9 files changed, 264 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/11/9211/7
--
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: newpatchset
Gerrit-Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9
Gerrit-Change-Number: 9211
Gerrit-PatchSet: 7
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Stefan Sperling 

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

[Neels Hofmeyr]

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 6:

(4 comments)

let me apply the cosmetics and merge...

https://gerrit.osmocom.org/#/c/9211/6/include/osmocom/msc/vlr.h
File include/osmocom/msc/vlr.h:

https://gerrit.osmocom.org/#/c/9211/6/include/osmocom/msc/vlr.h@23
PS6, Line 23: #define VLR_SUBSCRIBER_NO_EXPIRATION  0x0
(hex doesn't really apply)


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

https://gerrit.osmocom.org/#/c/9211/6/src/libvlr/vlr.c@478
PS6, Line 478:  vsub->expire_lu = VLR_SUBSCRIBER_NO_EXPIRATION;
I still don't like to introduce a backdoor that allows subscribers to never 
expire by fudging about with the server's clock. But yeah, if no-one else 
shares that view... I don't expect it to apply in reality, but if it ever does, 
I'll say "I told you so" ;)


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

https://gerrit.osmocom.org/#/c/9211/6/src/libvlr/vlr_lu_fsm.c@359
PS6, Line 359:  /* Balanced by vlr_subscr_rx_imsi_detach() or Location 
Update expiry */
still prefer "by vlr_subscr_expire()", which is accurate for both cases and 
pinpoints the actual place in the code.


https://gerrit.osmocom.org/#/c/9211/6/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/6/tests/msc_vlr/msc_vlr_test_no_authen.c@952
PS6, Line 952:  btw("having received subscriber data does not mean acceptance");
(these few lines are actually not related to the timeout and could be dropped, 
knowing that they are already checked in the other test)



-- 
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: 6
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Stefan Sperling 
Gerrit-Comment-Date: Wed, 23 May 2018 12:55:19 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No

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

[Stefan Sperling]

Hello Neels Hofmeyr, Harald Welte, Jenkins Builder,

I'd like you to reexamine a change. Please visit

https://gerrit.osmocom.org/9211

to look at the new patch set (#6).

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

implement periodic Location Update expiry in the VLR

Remove subscribers which fail to send periodic Location Updates from the
list of subscribers known to the VLR. This complements the IMSI detach
procedure: periodic LU expiry triggers an implicit IMSI detach.

Expired subscribers are purged from a periodic timer which iterates
over all subscribers once per minute.

Subscribers with an active connection do not expire. This is controlled
by the subscriber conn FSM which sets a subscriber's the LU expiry timeout
value to GSM_SUBSCRIBER_NO_EXPIRATION while a connection is active.

Add support for fake time with osmo_clock_gettime() to msc_vlr tests.

This functionality existed in OpenBSC but was lost during the nitb split.
This code took some inspiration from the OpenBSC implementation.

Related: OS#1976
Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9
---
M include/osmocom/msc/gsm_subscriber.h
M include/osmocom/msc/vlr.h
M src/libmsc/subscr_conn.c
M src/libvlr/vlr.c
M src/libvlr/vlr_lu_fsm.c
M tests/msc_vlr/msc_vlr_test_no_authen.c
M tests/msc_vlr/msc_vlr_test_no_authen.err
M tests/msc_vlr/msc_vlr_tests.c
M tests/msc_vlr/msc_vlr_tests.h
9 files changed, 283 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/11/9211/6
--
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: newpatchset
Gerrit-Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9
Gerrit-Change-Number: 9211
Gerrit-PatchSet: 6
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Stefan Sperling 

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

[Stefan Sperling]

Hello Neels Hofmeyr, Harald Welte, Jenkins Builder,

I'd like you to reexamine a change. Please visit

https://gerrit.osmocom.org/9211

to look at the new patch set (#5).

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

implement periodic Location Update expiry in the VLR

Remove subscribers which fail to send periodic Location Updates from the
list of subscribers known to the VLR. This complements the IMSI detach
procedure: periodic LU expiry triggers an implicit IMSI detach.

Expired subscribers are purged from a periodic timer which iterates
over all subscribers once per minute.

Subscribers with an active connection do not expire. This is controlled
by the subscriber conn FSM which sets a subscriber's the LU expiry timeout
value to GSM_SUBSCRIBER_NO_EXPIRATION while a connection is active.

Add support for fake time with osmo_clock_gettime() to msc_vlr tests.

This functionality existed in OpenBSC but was lost during the nitb split.
This code took some inspiration from the OpenBSC implementation.

Related: OS#1976
Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9
---
M include/osmocom/msc/gsm_subscriber.h
M include/osmocom/msc/vlr.h
M src/libmsc/subscr_conn.c
M src/libvlr/vlr.c
M src/libvlr/vlr_lu_fsm.c
M tests/msc_vlr/msc_vlr_test_no_authen.c
M tests/msc_vlr/msc_vlr_test_no_authen.err
M tests/msc_vlr/msc_vlr_tests.c
M tests/msc_vlr/msc_vlr_tests.h
9 files changed, 284 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/11/9211/5
--
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: newpatchset
Gerrit-Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9
Gerrit-Change-Number: 9211
Gerrit-PatchSet: 5
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Stefan Sperling 

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

[Stefan Sperling]

Hello Neels Hofmeyr, Harald Welte, Jenkins Builder,

I'd like you to reexamine a change. Please visit

https://gerrit.osmocom.org/9211

to look at the new patch set (#3).

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

implement periodic Location Update expiry in the VLR

Remove subscribers which fail to send periodic Location Updates from the
list of subscribers known to the VLR. This complements the IMSI detach
procedure: periodic LU expiry triggers an implicit IMSI detach.

Expired subscribers are purged from a periodic timer which iterates
over all subscribers once per minute.

Subscribers with an active connection do not expire. This is controlled
by the subscriber conn FSM which sets a subscriber's the LU expiry timeout
value to GSM_SUBSCRIBER_NO_EXPIRATION while a connection is active.

Add support for fake time with osmo_clock_gettime() to msc_vlr tests.

Related: OS#1976
Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9
---
M include/osmocom/msc/gsm_subscriber.h
M include/osmocom/msc/vlr.h
M src/libmsc/subscr_conn.c
M src/libvlr/vlr.c
M src/libvlr/vlr_lu_fsm.c
M tests/msc_vlr/msc_vlr_test_no_authen.c
M tests/msc_vlr/msc_vlr_test_no_authen.err
M tests/msc_vlr/msc_vlr_tests.c
M tests/msc_vlr/msc_vlr_tests.h
9 files changed, 281 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/11/9211/3
--
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: newpatchset
Gerrit-Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9
Gerrit-Change-Number: 9211
Gerrit-PatchSet: 3
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Stefan Sperling 

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

[Stefan Sperling]

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

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


Patch Set 2:

(4 comments)

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. […]
This comment was moved here, I didn't write it.
You don't seem to like it. Should I just delete it?


https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@481
PS2, Line 481:  vsub->expire_lu = GSM_SUBSCRIBER_NO_EXPIRATION;
> hmm. […]
I am not sure. This error really should not happen.

I suppose leaving subscribers in memory is better than kicking off all 
subscribers in case of a problem with reading the clock.


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(). […]
Sure.


https://gerrit.osmocom.org/#/c/9211/2/src/libvlr/vlr.c@1192
PS2, Line 1192: osmo_timer_setup(>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)
It's not in the same function.

Looks like gerrit has fooled you by leaving out some lines in-between...



--
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 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: Stefan Sperling 
Gerrit-Comment-Date: Fri, 18 May 2018 10:08:39 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No

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

[Neels Hofmeyr]

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(>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: 

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

[Stefan Sperling]

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

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


Patch Set 2:

(1 comment)

https://gerrit.osmocom.org/#/c/9211/2/src/libmsc/subscr_conn.c
File src/libmsc/subscr_conn.c:

https://gerrit.osmocom.org/#/c/9211/2/src/libmsc/subscr_conn.c@211
PS2, Line 211:   * The LU expiry timer will restart once the connection is 
closed. */
> do we need an extra field for this?  Doesn't every subscribe with active 
> connections have subscr->co […]
Yes we need an extra field.

subscr->conn != NULL is not a sufficient condition.

We want to stop the expiry timer only if the connection FSM makes it all the 
way into ACCEPTED state. If a connection does not enter ACCEPTED state the 
timer should keep ticking.
E.g. we don't want failed location updates to reset this timer.



--
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 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Stefan Sperling 
Gerrit-Comment-Date: Thu, 17 May 2018 12:22:56 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No

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

[Harald Welte]

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

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


Patch Set 2:

(1 comment)

https://gerrit.osmocom.org/#/c/9211/2/src/libmsc/subscr_conn.c
File src/libmsc/subscr_conn.c:

https://gerrit.osmocom.org/#/c/9211/2/src/libmsc/subscr_conn.c@211
PS2, Line 211:   * The LU expiry timer will restart once the connection is 
closed. */
do we need an extra field for this?  Doesn't every subscribe with active 
connections have subscr->conn != NULL and we can simply check on that?  It's 
not a strong argument, but if we have the information already somewhere, we 
shouldn't duplicate it.  Or are there other reasons to stop/halt/pause 
expirtation?



--
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 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Stefan Sperling 
Gerrit-Comment-Date: Thu, 17 May 2018 12:02:11 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No

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

[Stefan Sperling]

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

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


Patch Set 1:

(4 comments)

https://gerrit.osmocom.org/#/c/9211/1/src/libmsc/subscr_conn.c
File src/libmsc/subscr_conn.c:

https://gerrit.osmocom.org/#/c/9211/1/src/libmsc/subscr_conn.c@207
PS1, Line 207:  /* Stop location update expiry for this subscriber. */
> the comment states what's easily deducted from the line below. […]
Next patch contains has a better comment.

We want to prevent LU expiry from interfering with subscribers with active 
connections.


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

https://gerrit.osmocom.org/#/c/9211/1/src/libvlr/vlr.c@465
PS1, Line 465: vlr_subscr_enable_expire_lu(struct vlr_subscr *vsub)
> we don't generally break lines ahead of the function name
Indeed. Fixed in next patch set.


https://gerrit.osmocom.org/#/c/9211/1/src/libvlr/vlr.c@475
PS1, Line 475:  if (osmo_gettimeofday(, NULL) == 0) {
> I think we had the discussion about gettimeofday already (think of time 
> adjustments due to ntpdate,  […]
I was using gettimeofday only because the msc_vlr tests didn't support fake 
time with clock_gettime() yet.

Fixed in next patch set.
Adding support for this to the test suite was pretty straightforward.


https://gerrit.osmocom.org/#/c/9211/1/src/libvlr/vlr.c@481
PS1, Line 481:  vsub->expire_lu = GSM_SUBSCRIBER_NO_EXPIRATION; /* XXX 
*/
> what does the XXX mean here?
This subscriber will have LU expiry disabled.

Better comment in next patch set.



--
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: 1
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Stefan Sperling 
Gerrit-Comment-Date: Thu, 17 May 2018 11:01:29 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No

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

[Stefan Sperling]

Hello Harald Welte, Jenkins Builder,

I'd like you to reexamine a change. Please visit

https://gerrit.osmocom.org/9211

to look at the new patch set (#2).

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

implement periodic Location Update expiry in the VLR

Remove subscribers which fail to send periodic Location Updates from the
list of subscribers known to the VLR. This complements the IMSI detach
procedure: periodic LU expiry triggers an implicit IMSI detach.

Expired subscribers are purged from a periodic timer which iterates
over all subscribers once per minute.

Subscribers with an active connection do not expire. This is controlled
by the subscriber conn FSM which sets a subscriber's the LU expiry timeout
value to GSM_SUBSCRIBER_NO_EXPIRATION while a connection is active.

Add support for fake time with osmo_clock_gettime() to msc_vlr tests.

Related: OS#1976
Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9
---
M include/osmocom/msc/gsm_subscriber.h
M include/osmocom/msc/vlr.h
M src/libmsc/subscr_conn.c
M src/libvlr/vlr.c
M src/libvlr/vlr_lu_fsm.c
M tests/msc_vlr/msc_vlr_test_no_authen.c
M tests/msc_vlr/msc_vlr_test_no_authen.err
M tests/msc_vlr/msc_vlr_tests.c
M tests/msc_vlr/msc_vlr_tests.h
9 files changed, 285 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/11/9211/2
--
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: newpatchset
Gerrit-Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9
Gerrit-Change-Number: 9211
Gerrit-PatchSet: 2
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder

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

[Harald Welte]

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

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


Patch Set 1: Code-Review-1

(4 comments)

https://gerrit.osmocom.org/#/c/9211/1/src/libmsc/subscr_conn.c
File src/libmsc/subscr_conn.c:

https://gerrit.osmocom.org/#/c/9211/1/src/libmsc/subscr_conn.c@207
PS1, Line 207:  /* Stop location update expiry for this subscriber. */
the comment states what's easily deducted from the line below.  What the 
comment doesn't say (and I don't understand): Why are we stopping expiration 
here?


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

https://gerrit.osmocom.org/#/c/9211/1/src/libvlr/vlr.c@465
PS1, Line 465: vlr_subscr_enable_expire_lu(struct vlr_subscr *vsub)
we don't generally break lines ahead of the function name


https://gerrit.osmocom.org/#/c/9211/1/src/libvlr/vlr.c@475
PS1, Line 475:  if (osmo_gettimeofday(, NULL) == 0) {
I think we had the discussion about gettimeofday already (think of time 
adjustments due to ntpdate, ...), especially on embedded systems without RTC, 
where at boot time we don't have a valid wall-clock time.


https://gerrit.osmocom.org/#/c/9211/1/src/libvlr/vlr.c@481
PS1, Line 481:  vsub->expire_lu = GSM_SUBSCRIBER_NO_EXPIRATION; /* XXX 
*/
what does the XXX mean here?



--
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: 1
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Comment-Date: Thu, 17 May 2018 09:46:14 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes

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

[Stefan Sperling]

Stefan Sperling has uploaded this change for review. ( 
https://gerrit.osmocom.org/9211


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

implement periodic Location Update expiry in the VLR

Remove subscribers which fail to send periodic Location Updates from the
list of subscribers known to the VLR. This complements the IMSI detach
procedure: periodic LU expiry triggers an implicit IMSI detach.

Expired subscribers are purged from a periodic timer which iterates
over all subscribers once per minute.

Subscribers with an active connection do not expire. This is controlled
by the subscriber conn FSM which sets a subscriber's the LU expiry timeout
value to GSM_SUBSCRIBER_NO_EXPIRATION while a connection is active.

Related: OS#1976
Change-Id: Iebdee8b12d22acfcfb265ee41e71cfc8d9eb3ba9
---
M include/osmocom/msc/gsm_subscriber.h
M include/osmocom/msc/vlr.h
M src/libmsc/subscr_conn.c
M src/libvlr/vlr.c
M src/libvlr/vlr_lu_fsm.c
M tests/msc_vlr/msc_vlr_test_no_authen.c
M tests/msc_vlr/msc_vlr_test_no_authen.err
7 files changed, 273 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/11/9211/1

diff --git a/include/osmocom/msc/gsm_subscriber.h 
b/include/osmocom/msc/gsm_subscriber.h
index 16e1037..50b4a23 100644
--- a/include/osmocom/msc/gsm_subscriber.h
+++ b/include/osmocom/msc/gsm_subscriber.h
@@ -18,6 +18,7 @@
 /* gprs_sgsn.h defines additional flags including and above bit 16 (0x1) */

 #define GSM_SUBSCRIBER_NO_EXPIRATION   0x0
+#define GSM_SUBSCRIBER_LU_EXPIRATION_INTERVAL  60  /* in seconds */

 enum gsm_subscriber_field {
GSM_SUBSCRIBER_IMSI,
diff --git a/include/osmocom/msc/vlr.h b/include/osmocom/msc/vlr.h
index 0a9ef6f..c647ddc 100644
--- a/include/osmocom/msc/vlr.h
+++ b/include/osmocom/msc/vlr.h
@@ -148,6 +148,7 @@
struct osmo_fsm_inst *proc_arq_fsm;

bool lu_complete;
+   time_t expire_lu;
 
void *msc_conn_ref;

@@ -237,6 +238,7 @@
struct llist_head operations;
struct gsup_client *gsup_client;
struct vlr_ops ops;
+   struct osmo_timer_list lu_expire_timer;
struct {
bool retrieve_imeisv_early;
bool retrieve_imeisv_ciphered;
@@ -379,6 +381,7 @@
  enum osmo_fsm_term_cause fsm_cause,
  uint8_t gsm48_cause);

+void vlr_subscr_enable_expire_lu(struct vlr_subscr *vsub);

 /* Process Acccess Request FSM */

diff --git a/src/libmsc/subscr_conn.c b/src/libmsc/subscr_conn.c
index 1b3b240..50d7be6 100644
--- a/src/libmsc/subscr_conn.c
+++ b/src/libmsc/subscr_conn.c
@@ -202,6 +202,11 @@

 static void subscr_conn_fsm_accepted_enter(struct osmo_fsm_inst *fi, uint32_t 
prev_state)
 {
+   struct gsm_subscriber_connection *conn = fi->priv;
+
+   /* Stop location update expiry for this subscriber. */
+   conn->vsub->expire_lu = GSM_SUBSCRIBER_NO_EXPIRATION;
+
if (!subscr_conn_fsm_has_active_transactions(fi))
osmo_fsm_inst_dispatch(fi, SUBSCR_CONN_E_UNUSED, NULL);
 }
@@ -278,6 +283,11 @@
/* Cancel all VLR FSMs, if any */
vlr_subscr_cancel_attach_fsm(conn->vsub, OSMO_FSM_TERM_ERROR, 
GSM48_REJECT_CONGESTION);

+   if (conn->vsub) {
+   /* Restart LU expiry for this subscriber. */
+   vlr_subscr_enable_expire_lu(conn->vsub);
+   }
+
/* If we're closing in a middle of a trans, we need to clean up */
trans_conn_closed(conn);

diff --git a/src/libvlr/vlr.c b/src/libvlr/vlr.c
index 2d232be..b5274ba 100644
--- a/src/libvlr/vlr.c
+++ b/src/libvlr/vlr.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -460,6 +461,54 @@
return 0;
 }

+void
+vlr_subscr_enable_expire_lu(struct vlr_subscr *vsub)
+{
+   struct gsm_network *net = vsub->vlr->user_ctx; /* XXX move t3212 into 
struct vlr_instance? */
+   struct timeval now;
+
+   /* Table 10.5.33: The T3212 timeout value is coded as the
+* binary representation of the timeout value for
+* periodic updating in decihours. Mark the subscriber as
+* inactive if it missed two consecutive location updates.
+* Timeout is twice the t3212 value plus one minute */
+   if (osmo_gettimeofday(, NULL) == 0) {
+   vsub->expire_lu = now.tv_sec + (net->t3212 * 60 * 6 * 2) + 60;
+   } else {
+   LOGP(DVLR, LOGL_ERROR,
+"IMSI=%s id=%llu: Could not enable Location Update expiry: 
unable to read current time\n",
+vsub->imsi, vsub->id);
+   vsub->expire_lu = GSM_SUBSCRIBER_NO_EXPIRATION; /* XXX */
+   }
+}
+
+void
+vlr_subscr_expire_lu(void *data)
+{
+   struct vlr_instance *vlr = data;
+   struct vlr_subscr *vsub, *vsub_tmp;
+   struct timeval now;
+
+   if (llist_empty(>subscribers))
+