Fix MS ending up with assigned imsi 000

The whole paging path and data structre is cleaned up.
New MS helpers ms_imsi_is_valid() and ms_paging_group() are introduced
to help in the process and keep implementation details inside GprsMs
class.

Related: OS#5303
Change-Id: I4c0838b26ede58e4b711410eee2a8e4f71e9414b

9 files changed, 40 insertions, 29 deletions

diff --git a/src/bts.cpp b/src/bts.cpp
index dc20259c..6fabc90c 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -1037,10 +1037,11 @@ int bts_rcv_ptcch_rach(struct gprs_rlcmac_bts *bts, const struct rach_ind_params
 	return 0;
 }
 
-void bts_snd_dl_ass(struct gprs_rlcmac_bts *bts, struct gprs_rlcmac_tbf *tbf, uint16_t pgroup)
+void bts_snd_dl_ass(struct gprs_rlcmac_bts *bts, struct gprs_rlcmac_tbf *tbf)
 {
 	uint8_t trx_no = tbf->trx->trx_no;
 	uint8_t ts_no = tbf->first_ts;
+	uint16_t pgroup = ms_paging_group(tbf_ms(tbf));
 	int plen;
 
 	LOGPTBF(tbf, LOGL_INFO, "TX: START Immediate Assignment Downlink (PCH)\n");
diff --git a/src/bts.h b/src/bts.h
index 3a58c636..f947a558 100644
--- a/src/bts.h
+++ b/src/bts.h
@@ -301,7 +301,7 @@ uint32_t bts_rfn_to_fn(const struct gprs_rlcmac_bts *bts, int32_t rfn);
 struct gprs_rlcmac_dl_tbf *bts_dl_tbf_by_tfi(struct gprs_rlcmac_bts *bts, uint8_t tfi, uint8_t trx, uint8_t ts);
 struct gprs_rlcmac_ul_tbf *bts_ul_tbf_by_tfi(struct gprs_rlcmac_bts *bts, uint8_t tfi, uint8_t trx, uint8_t ts);
 
-void bts_snd_dl_ass(struct gprs_rlcmac_bts *bts, struct gprs_rlcmac_tbf *tbf, uint16_t pgroup);
+void bts_snd_dl_ass(struct gprs_rlcmac_bts *bts, struct gprs_rlcmac_tbf *tbf);
 
 void bts_set_current_frame_number(struct gprs_rlcmac_bts *bts, uint32_t frame_number);
 void bts_set_current_block_frame_number(struct gprs_rlcmac_bts *bts, int frame_number);
diff --git a/src/gprs_bssgp_pcu.c b/src/gprs_bssgp_pcu.c
index 4328e079..8cb53027 100644
--- a/src/gprs_bssgp_pcu.c
+++ b/src/gprs_bssgp_pcu.c
@@ -108,12 +108,8 @@ static int gprs_bssgp_pcu_rx_dl_ud(struct msgb *msg, struct tlv_parsed *tp)
 	uint8_t egprs_ms_class = 0;
 	int rc;
 	MS_Radio_Access_capability_t rac;
-	/* TODO: is it really necessary to initialize this as a "000" IMSI? It seems, the function should just return an
-	 * error if no IMSI IE was found. */
-	struct osmo_mobile_identity mi_imsi = {
-		.type = GSM_MI_TYPE_TMSI,
-	};
-	OSMO_STRLCPY_ARRAY(mi_imsi.imsi, "000");
+	const char *imsi = NULL;
+	struct osmo_mobile_identity mi_imsi;
 
 	budh = (struct bssgp_ud_hdr *)msgb_bssgph(msg);
 	tlli = ntohl(budh->tlli);
@@ -144,6 +140,7 @@ static int gprs_bssgp_pcu_rx_dl_ud(struct msgb *msg, struct tlv_parsed *tp)
 			LOGP(DBSSGP, LOGL_NOTICE, "Failed to parse IMSI IE (rc=%d)\n", rc);
 			return bssgp_tx_status(BSSGP_CAUSE_COND_IE_ERR, NULL, msg);
 		}
+		imsi = &mi_imsi.imsi[0];
 	}
 
 	/* parse ms radio access capability */
@@ -180,10 +177,11 @@ static int gprs_bssgp_pcu_rx_dl_ud(struct msgb *msg, struct tlv_parsed *tp)
 				"TLLI (old) IE\n");
 	}
 
-	LOGP(DBSSGP, LOGL_INFO, "LLC [SGSN -> PCU] = TLLI: 0x%08x IMSI: %s len: %d\n", tlli, mi_imsi.imsi, len);
+	LOGP(DBSSGP, LOGL_INFO, "LLC [SGSN -> PCU] = TLLI: 0x%08x IMSI: %s len: %d\n",
+	     tlli, imsi ? : "none", len);
 
-	return dl_tbf_handle(the_pcu->bssgp.bts, tlli, tlli_old, mi_imsi.imsi,
-			ms_class, egprs_ms_class, delay_csec, data, len);
+	return dl_tbf_handle(the_pcu->bssgp.bts, tlli, tlli_old, imsi, ms_class,
+			     egprs_ms_class, delay_csec, data, len);
 }
 
 /* 3GPP TS 48.018 Table 10.3.2. Returns 0 on success, suggested BSSGP cause otherwise */
diff --git a/src/gprs_ms.c b/src/gprs_ms.c
index 0d6be4d5..5e75d067 100644
--- a/src/gprs_ms.c
+++ b/src/gprs_ms.c
@@ -522,6 +522,18 @@ void ms_set_imsi(struct GprsMs *ms, const char *imsi)
 	osmo_strlcpy(ms->imsi, imsi, sizeof(ms->imsi));
 }
 
+uint16_t ms_paging_group(struct GprsMs *ms)
+{
+	uint16_t pgroup;
+	if (!ms_imsi_is_valid(ms))
+		return 0; /* 000 is the special "all paging" group */
+	if ((pgroup = imsi2paging_group(ms_imsi(ms))) > 999) {
+		LOGPMS(ms, DRLCMAC, LOGL_ERROR, "IMSI to paging group failed!\n");
+		return 0;
+	}
+	return pgroup;
+}
+
 void ms_set_ta(struct GprsMs *ms, uint8_t ta_)
 {
 	if (ta_ == ms->ta)
diff --git a/src/gprs_ms.h b/src/gprs_ms.h
index 4438a4c9..c579cf5a 100644
--- a/src/gprs_ms.h
+++ b/src/gprs_ms.h
@@ -132,6 +132,7 @@ void ms_detach_tbf(struct GprsMs *ms, struct gprs_rlcmac_tbf *tbf);
 void ms_set_tlli(struct GprsMs *ms, uint32_t tlli);
 bool ms_confirm_tlli(struct GprsMs *ms, uint32_t tlli);
 void ms_set_imsi(struct GprsMs *ms, const char *imsi);
+uint16_t ms_paging_group(struct GprsMs *ms);
 
 void ms_update_l1_meas(struct GprsMs *ms, const struct pcu_l1_meas *meas);
 
@@ -186,6 +187,11 @@ static inline const char *ms_imsi(const struct GprsMs *ms)
 	return ms->imsi;
 }
 
+static inline bool ms_imsi_is_valid(const struct GprsMs *ms)
+{
+	return ms->imsi[0] != '\0';
+}
+
 static inline uint8_t ms_ta(const struct GprsMs *ms)
 {
 	return ms->ta;
diff --git a/src/gprs_ms_storage.cpp b/src/gprs_ms_storage.cpp
index 6245ed9f..db3a7f96 100644
--- a/src/gprs_ms_storage.cpp
+++ b/src/gprs_ms_storage.cpp
@@ -29,8 +29,6 @@ extern "C" {
 	#include <osmocom/gsm/gsm48.h>
 }
 
-#define GPRS_UNDEFINED_IMSI "000"
-
 static void ms_storage_ms_idle_cb(struct GprsMs *ms)
 {
 	llist_del(&ms->list);
@@ -89,10 +87,10 @@ GprsMs *GprsMsStorage::get_ms(uint32_t tlli, uint32_t old_tlli, const char *imsi
 
 	/* not found by TLLI */
 
-	if (imsi && imsi[0] && strcmp(imsi, GPRS_UNDEFINED_IMSI) != 0) {
+	if (imsi && imsi[0] != '\0') {
 		llist_for_each(tmp, &m_list) {
 			ms = llist_entry(tmp, typeof(*ms), list);
-			if (strcmp(imsi, ms_imsi(ms)) == 0)
+			if (ms_imsi_is_valid(ms) && strcmp(imsi, ms_imsi(ms)) == 0)
 				return ms;
 		}
 	}
diff --git a/src/tbf_dl.cpp b/src/tbf_dl.cpp
index 05d5ad36..6e898dad 100644
--- a/src/tbf_dl.cpp
+++ b/src/tbf_dl.cpp
@@ -296,7 +296,9 @@ int dl_tbf_handle(struct gprs_rlcmac_bts *bts,
 	/* check for existing TBF */
 	ms = bts_ms_store(bts)->get_ms(tlli, tlli_old, imsi);
 
-	if (ms && strlen(ms_imsi(ms)) == 0) {
+	/* If we got MS by TLLI above let's see if we already have another MS
+	 * object identified by IMSI and merge them */
+	if (ms && !ms_imsi_is_valid(ms) && imsi) {
 		ms_old = bts_ms_store(bts)->get_ms(0, 0, imsi);
 		if (ms_old && ms_old != ms) {
 			/* The TLLI has changed (RAU), so there are two MS
@@ -310,7 +312,7 @@ int dl_tbf_handle(struct gprs_rlcmac_bts *bts,
 			if (!ms_dl_tbf(ms) && ms_dl_tbf(ms_old)) {
 				LOGP(DTBF, LOGL_NOTICE,
 				     "IMSI %s, old TBF %s: moving DL TBF to new MS object\n",
-				     imsi, ms_dl_tbf(ms_old)->name());
+				     imsi ? : "unknown", ms_dl_tbf(ms_old)->name());
 				dl_tbf = ms_dl_tbf(ms_old);
 				/* Move the DL TBF to the new MS */
 				dl_tbf->set_ms(ms);
@@ -323,7 +325,8 @@ int dl_tbf_handle(struct gprs_rlcmac_bts *bts,
 
 	if (!ms)
 		ms = bts_alloc_ms(bts, ms_class, egprs_ms_class);
-	ms_set_imsi(ms, imsi);
+	if (imsi)
+		ms_set_imsi(ms, imsi);
 	ms_confirm_tlli(ms, tlli);
 	if (!ms_ms_class(ms) && ms_class) {
 		ms_set_ms_class(ms, ms_class);
@@ -599,7 +602,6 @@ struct msgb *gprs_rlcmac_dl_tbf::create_dl_acked_block(uint32_t fn, uint8_t ts,
 /* depending on the current TBF, we assign on PACCH or AGCH */
 void gprs_rlcmac_dl_tbf::trigger_ass(struct gprs_rlcmac_tbf *old_tbf)
 {
-	uint16_t pgroup;
 	/* stop pending timer */
 	stop_timers("assignment (DL-TBF)");
 
@@ -618,9 +620,7 @@ void gprs_rlcmac_dl_tbf::trigger_ass(struct gprs_rlcmac_tbf *old_tbf)
 		osmo_fsm_inst_dispatch(this->state_fsm.fi, TBF_EV_ASSIGN_ADD_CCCH, NULL);
 
 		/* send immediate assignment */
-		if ((pgroup = imsi2paging_group(imsi())) > 999)
-			LOGPTBFDL(this, LOGL_ERROR, "IMSI to paging group failed! (%s)\n", imsi());
-		bts_snd_dl_ass(bts, this, pgroup);
+		bts_snd_dl_ass(bts, this);
 	}
 }
 
diff --git a/src/tbf_fsm.c b/src/tbf_fsm.c
index af2b34ef..39f20805 100644
--- a/src/tbf_fsm.c
+++ b/src/tbf_fsm.c
@@ -214,15 +214,11 @@ static void st_flow(struct osmo_fsm_inst *fi, uint32_t event, void *data)
 		if ((ctx->state_flags & (1 << GPRS_RLCMAC_FLAG_CCCH))
 		     && !(ctx->state_flags & (1 << GPRS_RLCMAC_FLAG_DL_ACK))) {
 			struct GprsMs *ms = tbf_ms(ctx->tbf);
-			const char *imsi = ms_imsi(ms);
-			uint16_t pgroup;
 			LOGPTBF(ctx->tbf, LOGL_DEBUG, "Re-send downlink assignment on PCH (IMSI=%s)\n",
-				imsi);
+				ms_imsi_is_valid(ms) ? ms_imsi(ms) : "");
 			tbf_fsm_state_chg(fi, TBF_ST_ASSIGN);
 			/* send immediate assignment */
-			if ((pgroup = imsi2paging_group(imsi)) > 999)
-				LOGPTBF(ctx->tbf, LOGL_ERROR, "IMSI to paging group failed! (%s)\n", imsi);
-			bts_snd_dl_ass(ms->bts, ctx->tbf, pgroup);
+			bts_snd_dl_ass(ms->bts, ctx->tbf);
 		}
 		break;
 	case TBF_EV_LAST_DL_DATA_SENT:
diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp
index 8700347d..9bea528b 100644
--- a/tests/tbf/TbfTest.cpp
+++ b/tests/tbf/TbfTest.cpp
@@ -1718,7 +1718,7 @@ static void send_dl_data(struct gprs_rlcmac_bts *bts, uint32_t tlli, const char
 	OSMO_ASSERT(ms != NULL);
 	OSMO_ASSERT(ms_dl_tbf(ms) != NULL);
 
-	if (imsi[0] && strcmp(imsi, "000") != 0) {
+	if (imsi[0] != '\0') {
 		ms2 = bts_ms_by_tlli(bts, tlli, GSM_RESERVED_TMSI);
 		OSMO_ASSERT(ms == ms2);
 	}