[MERGED] osmo-msc[master]: fix: properly cancel all Paging on IMSI Detach

[Harald Welte]

Harald Welte has submitted this change and it was merged.

Change subject: fix: properly cancel all Paging on IMSI Detach
..


fix: properly cancel all Paging on IMSI Detach

It's not clear cut which code is responsible for canceling pending requests,
since the requests list is kept in vlr_subscr, but sending out Paging does
certainly not belong in the VLR. Place the requests cleanup in gsm_04_08.c.

Add to test_ms_timeout_paging() in msc_vlr_test_ms_timeout.c to verify that a
pending paging is canceled on IMSI Detach.

Change-Id: Ib8874a9d92f02b0826525b55518332f6899688fd
---
M include/osmocom/msc/gsm_subscriber.h
M src/libmsc/gsm_04_08.c
M src/libmsc/gsm_subscriber.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.err
5 files changed, 43 insertions(+), 5 deletions(-)

Approvals:
  Harald Welte: 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 6b0e3da..4adfd41 100644
--- a/include/osmocom/msc/gsm_subscriber.h
+++ b/include/osmocom/msc/gsm_subscriber.h
@@ -62,6 +62,7 @@
 int subscr_rx_paging_response(struct msgb *msg,
  struct gsm_subscriber_connection *conn);
 
+void subscr_paging_cancel(struct vlr_subscr *vsub, enum gsm_paging_event 
event);
 int subscr_paging_dispatch(unsigned int hooknum, unsigned int event,
   struct msgb *msg, void *data, void *param);
 
diff --git a/src/libmsc/gsm_04_08.c b/src/libmsc/gsm_04_08.c
index e370962..26ac923 100644
--- a/src/libmsc/gsm_04_08.c
+++ b/src/libmsc/gsm_04_08.c
@@ -770,6 +770,10 @@
 gsm48_mi_type_name(mi_type), mi_string);
} else {
LOGP(DMM, LOGL_INFO, "IMSI DETACH for %s\n", 
vlr_subscr_name(vsub));
+
+   if (vsub->cs.is_paging)
+   subscr_paging_cancel(vsub, GSM_PAGING_EXPIRED);
+
vlr_subscr_rx_imsi_detach(vsub);
osmo_signal_dispatch(SS_SUBSCR, S_SUBSCR_DETACHED, vsub);
vlr_subscr_put(vsub);
diff --git a/src/libmsc/gsm_subscriber.c b/src/libmsc/gsm_subscriber.c
index b3d38d1..1a7bf59 100644
--- a/src/libmsc/gsm_subscriber.c
+++ b/src/libmsc/gsm_subscriber.c
@@ -52,6 +52,11 @@
 #include 
 #include 
 
+void subscr_paging_cancel(struct vlr_subscr *vsub, enum gsm_paging_event event)
+{
+   subscr_paging_dispatch(GSM_HOOK_RR_PAGING, event, NULL, NULL, vsub);
+}
+
 int subscr_paging_dispatch(unsigned int hooknum, unsigned int event,
   struct msgb *msg, void *data, void *param)
 {
@@ -132,7 +137,7 @@
 static void paging_response_timer_cb(void *data)
 {
struct vlr_subscr *vsub = data;
-   subscr_paging_dispatch(GSM_HOOK_RR_PAGING, GSM_PAGING_EXPIRED, NULL, 
NULL, vsub);
+   subscr_paging_cancel(vsub, GSM_PAGING_EXPIRED);
 }
 
 /*! \brief Start a paging request for vsub, call cbfn(param) when done.
diff --git a/tests/msc_vlr/msc_vlr_test_ms_timeout.c 
b/tests/msc_vlr/msc_vlr_test_ms_timeout.c
index 4e0e27d..4cfd035 100644
--- a/tests/msc_vlr/msc_vlr_test_ms_timeout.c
+++ b/tests/msc_vlr/msc_vlr_test_ms_timeout.c
@@ -258,13 +258,26 @@
OSMO_ASSERT(vsub);
VERBOSE_ASSERT(vsub->cs.is_paging, == false, "%d");
VERBOSE_ASSERT(llist_count(>cs.requests), == 0, "%d");
+
+   BTW("Now that the timeout has expired, another Paging is sent on 
request");
+   paging_expect_imsi(imsi);
+   paging_sent = false;
+
+   send_sms(vsub, vsub,
+"Privacy in residential applications is a desirable"
+" marketing option.");
+
+   VERBOSE_ASSERT(llist_count(>cs.requests), == 1, "%d");
vlr_subscr_put(vsub);
vsub = NULL;
+   VERBOSE_ASSERT(paging_sent, == true, "%d");
+   VERBOSE_ASSERT(paging_stopped, == false, "%d");
 
-   BTW("subscriber detaches");
+   BTW("subscriber detaches, pagings are canceled");
expect_bssap_clear();
ms_sends_msg("05013008991007006402");
VERBOSE_ASSERT(bssap_clear_sent, == true, "%d");
+   VERBOSE_ASSERT(paging_stopped, == true, "%d");
 
vsub = vlr_subscr_find_by_imsi(net->vlr, imsi);
OSMO_ASSERT(!vsub);
diff --git a/tests/msc_vlr/msc_vlr_test_ms_timeout.err 
b/tests/msc_vlr/msc_vlr_test_ms_timeout.err
index c8b7e96..f0c4116 100644
--- a/tests/msc_vlr/msc_vlr_test_ms_timeout.err
+++ b/tests/msc_vlr/msc_vlr_test_ms_timeout.err
@@ -480,16 +480,30 @@
 DREF VLR subscr MSISDN:46071 usage increases to: 2
   vsub->cs.is_paging == 0
   llist_count(>cs.requests) == 0
-DREF VLR subscr MSISDN:46071 usage decreases to: 1
 ---
-- subscriber detaches
+- Now that the timeout has expired, another Paging is sent on request
+DREF VLR subscr MSISDN:46071 usage increases to: 3
+DMM Subscriber MSISDN:46071 not paged yet, start paging.
+  RAN_GERAN_A sends out paging request to IMSI 90170004620, TMSI 
0x, LAC 

osmo-msc[master]: fix: properly cancel all Paging on IMSI Detach

[Harald Welte]

Patch Set 3: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/5464
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8874a9d92f02b0826525b55518332f6899688fd
Gerrit-PatchSet: 3
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-HasComments: No

[PATCH] osmo-msc[master]: fix: properly cancel all Paging on IMSI Detach

[Neels Hofmeyr]

Review at  https://gerrit.osmocom.org/5464

fix: properly cancel all Paging on IMSI Detach

It's not clear cut which code is responsible for canceling pending requests,
since the requests list is kept in vlr_subscr, but sending out Paging does
certainly not belong in the VLR. Place the requests cleanup in gsm_04_08.c.

Add to test_ms_timeout_paging() in msc_vlr_test_ms_timeout.c to verify that a
pending paging is canceled on IMSI Detach.

Change-Id: Ib8874a9d92f02b0826525b55518332f6899688fd
---
M include/osmocom/msc/gsm_subscriber.h
M src/libmsc/gsm_04_08.c
M src/libmsc/gsm_subscriber.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.err
5 files changed, 43 insertions(+), 5 deletions(-)


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

diff --git a/include/osmocom/msc/gsm_subscriber.h 
b/include/osmocom/msc/gsm_subscriber.h
index 6b0e3da..4adfd41 100644
--- a/include/osmocom/msc/gsm_subscriber.h
+++ b/include/osmocom/msc/gsm_subscriber.h
@@ -62,6 +62,7 @@
 int subscr_rx_paging_response(struct msgb *msg,
  struct gsm_subscriber_connection *conn);
 
+void subscr_paging_cancel(struct vlr_subscr *vsub, enum gsm_paging_event 
event);
 int subscr_paging_dispatch(unsigned int hooknum, unsigned int event,
   struct msgb *msg, void *data, void *param);
 
diff --git a/src/libmsc/gsm_04_08.c b/src/libmsc/gsm_04_08.c
index d71b48b..14e7146 100644
--- a/src/libmsc/gsm_04_08.c
+++ b/src/libmsc/gsm_04_08.c
@@ -770,6 +770,10 @@
 gsm48_mi_type_name(mi_type), mi_string);
} else {
LOGP(DMM, LOGL_INFO, "IMSI DETACH for %s\n", 
vlr_subscr_name(vsub));
+
+   if (vsub->cs.is_paging)
+   subscr_paging_cancel(vsub, GSM_PAGING_EXPIRED);
+
vlr_subscr_rx_imsi_detach(vsub);
osmo_signal_dispatch(SS_SUBSCR, S_SUBSCR_DETACHED, vsub);
vlr_subscr_put(vsub);
diff --git a/src/libmsc/gsm_subscriber.c b/src/libmsc/gsm_subscriber.c
index b534b3f..4eaf164 100644
--- a/src/libmsc/gsm_subscriber.c
+++ b/src/libmsc/gsm_subscriber.c
@@ -52,6 +52,11 @@
 #include 
 #include 
 
+void subscr_paging_cancel(struct vlr_subscr *vsub, enum gsm_paging_event event)
+{
+   subscr_paging_dispatch(GSM_HOOK_RR_PAGING, event, NULL, NULL, vsub);
+}
+
 int subscr_paging_dispatch(unsigned int hooknum, unsigned int event,
   struct msgb *msg, void *data, void *param)
 {
@@ -132,7 +137,7 @@
 static void paging_timeout_cb(void *data)
 {
struct vlr_subscr *vsub = data;
-   subscr_paging_dispatch(GSM_HOOK_RR_PAGING, GSM_PAGING_EXPIRED, NULL, 
NULL, vsub);
+   subscr_paging_cancel(vsub, GSM_PAGING_EXPIRED);
 }
 
 /*! \brief Start a paging request for vsub, call cbfn(param) when done.
diff --git a/tests/msc_vlr/msc_vlr_test_ms_timeout.c 
b/tests/msc_vlr/msc_vlr_test_ms_timeout.c
index be11456..6aaf329 100644
--- a/tests/msc_vlr/msc_vlr_test_ms_timeout.c
+++ b/tests/msc_vlr/msc_vlr_test_ms_timeout.c
@@ -258,13 +258,26 @@
OSMO_ASSERT(vsub);
VERBOSE_ASSERT(vsub->cs.is_paging, == false, "%d");
VERBOSE_ASSERT(llist_count(>cs.requests), == 0, "%d");
+
+   BTW("Now that the timeout has expired, another Paging is sent on 
request");
+   paging_expect_imsi(imsi);
+   paging_sent = false;
+
+   send_sms(vsub, vsub,
+"Privacy in residential applications is a desirable"
+" marketing option.");
+
+   VERBOSE_ASSERT(llist_count(>cs.requests), == 1, "%d");
vlr_subscr_put(vsub);
vsub = NULL;
+   VERBOSE_ASSERT(paging_sent, == true, "%d");
+   VERBOSE_ASSERT(paging_stopped, == false, "%d");
 
-   BTW("subscriber detaches");
+   BTW("subscriber detaches, pagings are canceled");
expect_bssap_clear();
ms_sends_msg("05013008991007006402");
VERBOSE_ASSERT(bssap_clear_sent, == true, "%d");
+   VERBOSE_ASSERT(paging_stopped, == true, "%d");
 
vsub = vlr_subscr_find_by_imsi(net->vlr, imsi);
OSMO_ASSERT(!vsub);
diff --git a/tests/msc_vlr/msc_vlr_test_ms_timeout.err 
b/tests/msc_vlr/msc_vlr_test_ms_timeout.err
index 2426e7d..f272b24 100644
--- a/tests/msc_vlr/msc_vlr_test_ms_timeout.err
+++ b/tests/msc_vlr/msc_vlr_test_ms_timeout.err
@@ -480,16 +480,30 @@
 DREF VLR subscr MSISDN:46071 usage increases to: 2
   vsub->cs.is_paging == 0
   llist_count(>cs.requests) == 0
-DREF VLR subscr MSISDN:46071 usage decreases to: 1
 ---
-- subscriber detaches
+- Now that the timeout has expired, another Paging is sent on request
+DREF VLR subscr MSISDN:46071 usage increases to: 3
+DMM Subscriber MSISDN:46071 not paged yet, start paging.
+  RAN_GERAN_A sends out paging request to IMSI 90170004620, TMSI 
0x, LAC 23
+  strcmp(paging_expecting_imsi, imsi) == 0
+DREF VLR subscr MSISDN:46071 usage increases to: 4
+  llist_count(>cs.requests) == 1
+DREF VLR subscr MSISDN:46071