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/.
pespin gerrit-no-reply at lists.osmocom.orgpespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/26222 )
Change subject: Fix MS ending up with assigned imsi 000
......................................................................
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
---
M src/bts.cpp
M src/bts.h
M src/gprs_bssgp_pcu.c
M src/gprs_ms.c
M src/gprs_ms.h
M src/gprs_ms_storage.cpp
M src/tbf_dl.cpp
M src/tbf_fsm.c
M tests/tbf/TbfTest.cpp
9 files changed, 40 insertions(+), 29 deletions(-)
Approvals:
Jenkins Builder: Verified
fixeria: Looks good to me, but someone else must approve
laforge: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
diff --git a/src/bts.cpp b/src/bts.cpp
index dc20259..6fabc90 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -1037,10 +1037,11 @@
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 3a58c63..f947a55 100644
--- a/src/bts.h
+++ b/src/bts.h
@@ -301,7 +301,7 @@
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 4328e07..8cb5302 100644
--- a/src/gprs_bssgp_pcu.c
+++ b/src/gprs_bssgp_pcu.c
@@ -108,12 +108,8 @@
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 @@
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 @@
"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 0d6be4d..5e75d06 100644
--- a/src/gprs_ms.c
+++ b/src/gprs_ms.c
@@ -522,6 +522,18 @@
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 4438a4c..c579cf5 100644
--- a/src/gprs_ms.h
+++ b/src/gprs_ms.h
@@ -132,6 +132,7 @@
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 @@
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 6245ed9..db3a7f9 100644
--- a/src/gprs_ms_storage.cpp
+++ b/src/gprs_ms_storage.cpp
@@ -29,8 +29,6 @@
#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 @@
/* 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 05d5ad3..6e898da 100644
--- a/src/tbf_dl.cpp
+++ b/src/tbf_dl.cpp
@@ -296,7 +296,9 @@
/* 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 @@
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 @@
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 @@
/* 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 @@
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 af2b34e..39f2080 100644
--- a/src/tbf_fsm.c
+++ b/src/tbf_fsm.c
@@ -214,15 +214,11 @@
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 8700347..9bea528 100644
--- a/tests/tbf/TbfTest.cpp
+++ b/tests/tbf/TbfTest.cpp
@@ -1718,7 +1718,7 @@
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);
}
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/26222
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I4c0838b26ede58e4b711410eee2a8e4f71e9414b
Gerrit-Change-Number: 26222
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211112/1fb67378/attachment.htm>