<p>pespin has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/26222">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">Fix MS ending up with assigned imsi 000<br><br>The whole paging path and data structre is cleaned up.<br>New MS helpers ms_imsi_is_valid() and ms_paging_group() are introduced<br>to help in the process and keep implementation details inside GprsMs<br>class.<br><br>Related: OS#5303<br>Change-Id: I4c0838b26ede58e4b711410eee2a8e4f71e9414b<br>---<br>M src/gprs_bssgp_pcu.c<br>M src/gprs_ms.c<br>M src/gprs_ms.h<br>M src/gprs_ms_storage.cpp<br>M src/tbf_dl.cpp<br>M tests/tbf/TbfTest.cpp<br>6 files changed, 35 insertions(+), 19 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/22/26222/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/gprs_bssgp_pcu.c b/src/gprs_bssgp_pcu.c</span><br><span>index 4328e07..8cb5302 100644</span><br><span>--- a/src/gprs_bssgp_pcu.c</span><br><span>+++ b/src/gprs_bssgp_pcu.c</span><br><span>@@ -108,12 +108,8 @@</span><br><span> uint8_t egprs_ms_class = 0;</span><br><span> int rc;</span><br><span> MS_Radio_Access_capability_t rac;</span><br><span style="color: hsl(0, 100%, 40%);">- /* TODO: is it really necessary to initialize this as a "000" IMSI? It seems, the function should just return an</span><br><span style="color: hsl(0, 100%, 40%);">- * error if no IMSI IE was found. */</span><br><span style="color: hsl(0, 100%, 40%);">- struct osmo_mobile_identity mi_imsi = {</span><br><span style="color: hsl(0, 100%, 40%);">- .type = GSM_MI_TYPE_TMSI,</span><br><span style="color: hsl(0, 100%, 40%);">- };</span><br><span style="color: hsl(0, 100%, 40%);">- OSMO_STRLCPY_ARRAY(mi_imsi.imsi, "000");</span><br><span style="color: hsl(120, 100%, 40%);">+ const char *imsi = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ struct osmo_mobile_identity mi_imsi;</span><br><span> </span><br><span> budh = (struct bssgp_ud_hdr *)msgb_bssgph(msg);</span><br><span> tlli = ntohl(budh->tlli);</span><br><span>@@ -144,6 +140,7 @@</span><br><span> LOGP(DBSSGP, LOGL_NOTICE, "Failed to parse IMSI IE (rc=%d)\n", rc);</span><br><span> return bssgp_tx_status(BSSGP_CAUSE_COND_IE_ERR, NULL, msg);</span><br><span> }</span><br><span style="color: hsl(120, 100%, 40%);">+ imsi = &mi_imsi.imsi[0];</span><br><span> }</span><br><span> </span><br><span> /* parse ms radio access capability */</span><br><span>@@ -180,10 +177,11 @@</span><br><span> "TLLI (old) IE\n");</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- LOGP(DBSSGP, LOGL_INFO, "LLC [SGSN -> PCU] = TLLI: 0x%08x IMSI: %s len: %d\n", tlli, mi_imsi.imsi, len);</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGP(DBSSGP, LOGL_INFO, "LLC [SGSN -> PCU] = TLLI: 0x%08x IMSI: %s len: %d\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ tlli, imsi ? : "none", len);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- return dl_tbf_handle(the_pcu->bssgp.bts, tlli, tlli_old, mi_imsi.imsi,</span><br><span style="color: hsl(0, 100%, 40%);">- ms_class, egprs_ms_class, delay_csec, data, len);</span><br><span style="color: hsl(120, 100%, 40%);">+ return dl_tbf_handle(the_pcu->bssgp.bts, tlli, tlli_old, imsi, ms_class,</span><br><span style="color: hsl(120, 100%, 40%);">+ egprs_ms_class, delay_csec, data, len);</span><br><span> }</span><br><span> </span><br><span> /* 3GPP TS 48.018 Table 10.3.2. Returns 0 on success, suggested BSSGP cause otherwise */</span><br><span>diff --git a/src/gprs_ms.c b/src/gprs_ms.c</span><br><span>index 0d6be4d..5e75d06 100644</span><br><span>--- a/src/gprs_ms.c</span><br><span>+++ b/src/gprs_ms.c</span><br><span>@@ -522,6 +522,18 @@</span><br><span> osmo_strlcpy(ms->imsi, imsi, sizeof(ms->imsi));</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+uint16_t ms_paging_group(struct GprsMs *ms)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ uint16_t pgroup;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!ms_imsi_is_valid(ms))</span><br><span style="color: hsl(120, 100%, 40%);">+ return 0; /* 000 is the special "all paging" group */</span><br><span style="color: hsl(120, 100%, 40%);">+ if ((pgroup = imsi2paging_group(ms_imsi(ms))) > 999) {</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGPMS(ms, DRLCMAC, LOGL_ERROR, "IMSI to paging group failed!\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ return 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ return pgroup;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> void ms_set_ta(struct GprsMs *ms, uint8_t ta_)</span><br><span> {</span><br><span> if (ta_ == ms->ta)</span><br><span>diff --git a/src/gprs_ms.h b/src/gprs_ms.h</span><br><span>index 4438a4c..c579cf5 100644</span><br><span>--- a/src/gprs_ms.h</span><br><span>+++ b/src/gprs_ms.h</span><br><span>@@ -132,6 +132,7 @@</span><br><span> void ms_set_tlli(struct GprsMs *ms, uint32_t tlli);</span><br><span> bool ms_confirm_tlli(struct GprsMs *ms, uint32_t tlli);</span><br><span> void ms_set_imsi(struct GprsMs *ms, const char *imsi);</span><br><span style="color: hsl(120, 100%, 40%);">+uint16_t ms_paging_group(struct GprsMs *ms);</span><br><span> </span><br><span> void ms_update_l1_meas(struct GprsMs *ms, const struct pcu_l1_meas *meas);</span><br><span> </span><br><span>@@ -186,6 +187,11 @@</span><br><span> return ms->imsi;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+static inline bool ms_imsi_is_valid(const struct GprsMs *ms)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ return ms->imsi[0] != '\0';</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> static inline uint8_t ms_ta(const struct GprsMs *ms)</span><br><span> {</span><br><span> return ms->ta;</span><br><span>diff --git a/src/gprs_ms_storage.cpp b/src/gprs_ms_storage.cpp</span><br><span>index 6245ed9..db3a7f9 100644</span><br><span>--- a/src/gprs_ms_storage.cpp</span><br><span>+++ b/src/gprs_ms_storage.cpp</span><br><span>@@ -29,8 +29,6 @@</span><br><span> #include <osmocom/gsm/gsm48.h></span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-#define GPRS_UNDEFINED_IMSI "000"</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> static void ms_storage_ms_idle_cb(struct GprsMs *ms)</span><br><span> {</span><br><span> llist_del(&ms->list);</span><br><span>@@ -89,10 +87,10 @@</span><br><span> </span><br><span> /* not found by TLLI */</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (imsi && imsi[0] && strcmp(imsi, GPRS_UNDEFINED_IMSI) != 0) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (imsi && imsi[0] != '\0') {</span><br><span> llist_for_each(tmp, &m_list) {</span><br><span> ms = llist_entry(tmp, typeof(*ms), list);</span><br><span style="color: hsl(0, 100%, 40%);">- if (strcmp(imsi, ms_imsi(ms)) == 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ms_imsi_is_valid(ms) && strcmp(imsi, ms_imsi(ms)) == 0)</span><br><span> return ms;</span><br><span> }</span><br><span> }</span><br><span>diff --git a/src/tbf_dl.cpp b/src/tbf_dl.cpp</span><br><span>index 05d5ad3..20811ac 100644</span><br><span>--- a/src/tbf_dl.cpp</span><br><span>+++ b/src/tbf_dl.cpp</span><br><span>@@ -296,7 +296,9 @@</span><br><span> /* check for existing TBF */</span><br><span> ms = bts_ms_store(bts)->get_ms(tlli, tlli_old, imsi);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (ms && strlen(ms_imsi(ms)) == 0) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* If we got MS by TLLI above let's see if we already have another MS</span><br><span style="color: hsl(120, 100%, 40%);">+ * object identified by IMSI and merge them */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ms && !ms_imsi_is_valid(ms) && imsi) {</span><br><span> ms_old = bts_ms_store(bts)->get_ms(0, 0, imsi);</span><br><span> if (ms_old && ms_old != ms) {</span><br><span> /* The TLLI has changed (RAU), so there are two MS</span><br><span>@@ -310,7 +312,7 @@</span><br><span> if (!ms_dl_tbf(ms) && ms_dl_tbf(ms_old)) {</span><br><span> LOGP(DTBF, LOGL_NOTICE,</span><br><span> "IMSI %s, old TBF %s: moving DL TBF to new MS object\n",</span><br><span style="color: hsl(0, 100%, 40%);">- imsi, ms_dl_tbf(ms_old)->name());</span><br><span style="color: hsl(120, 100%, 40%);">+ imsi ? : "unknown", ms_dl_tbf(ms_old)->name());</span><br><span> dl_tbf = ms_dl_tbf(ms_old);</span><br><span> /* Move the DL TBF to the new MS */</span><br><span> dl_tbf->set_ms(ms);</span><br><span>@@ -323,7 +325,8 @@</span><br><span> </span><br><span> if (!ms)</span><br><span> ms = bts_alloc_ms(bts, ms_class, egprs_ms_class);</span><br><span style="color: hsl(0, 100%, 40%);">- ms_set_imsi(ms, imsi);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (imsi)</span><br><span style="color: hsl(120, 100%, 40%);">+ ms_set_imsi(ms, imsi);</span><br><span> ms_confirm_tlli(ms, tlli);</span><br><span> if (!ms_ms_class(ms) && ms_class) {</span><br><span> ms_set_ms_class(ms, ms_class);</span><br><span>@@ -618,8 +621,7 @@</span><br><span> osmo_fsm_inst_dispatch(this->state_fsm.fi, TBF_EV_ASSIGN_ADD_CCCH, NULL);</span><br><span> </span><br><span> /* send immediate assignment */</span><br><span style="color: hsl(0, 100%, 40%);">- if ((pgroup = imsi2paging_group(imsi())) > 999)</span><br><span style="color: hsl(0, 100%, 40%);">- LOGPTBFDL(this, LOGL_ERROR, "IMSI to paging group failed! (%s)\n", imsi());</span><br><span style="color: hsl(120, 100%, 40%);">+ pgroup = ms_paging_group(ms());</span><br><span> bts_snd_dl_ass(bts, this, pgroup);</span><br><span> }</span><br><span> }</span><br><span>diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp</span><br><span>index 8700347..9bea528 100644</span><br><span>--- a/tests/tbf/TbfTest.cpp</span><br><span>+++ b/tests/tbf/TbfTest.cpp</span><br><span>@@ -1718,7 +1718,7 @@</span><br><span> OSMO_ASSERT(ms != NULL);</span><br><span> OSMO_ASSERT(ms_dl_tbf(ms) != NULL);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (imsi[0] && strcmp(imsi, "000") != 0) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (imsi[0] != '\0') {</span><br><span> ms2 = bts_ms_by_tlli(bts, tlli, GSM_RESERVED_TMSI);</span><br><span> OSMO_ASSERT(ms == ms2);</span><br><span> }</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/26222">change 26222</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/c/osmo-pcu/+/26222"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-pcu </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I4c0838b26ede58e4b711410eee2a8e4f71e9414b </div>
<div style="display:none"> Gerrit-Change-Number: 26222 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>