ms: Make ms_{attach,detach}_tbf expectancies more robust
* Make sure that the tbf being attached has already the MS assigned.
* Check no re-attaching of alredy attached TBF ever happens.
* Document and early skip case where a non-attached TBF detach is
attempted.
* Avoid recursive call mess to tbf_set_ms(tbf, NULL); during detach.
* MsTest needs to be modified since it uses fake TBF objects which use
different set of calls than the regular TBFs in osmo-pcu. Since the
ul_tbf object is reused, it needs to be assigned ul_tbf->ms again before
re-assigning it, as per what happens usually in tbf_set_ms() in
osmo-pcu.
Change-Id: Ia18fe2de1fb3bf72f530157e5f30de64f2b11e12
diff --git a/src/gprs_ms.c b/src/gprs_ms.c
index 357470b..da50426 100644
--- a/src/gprs_ms.c
+++ b/src/gprs_ms.c
@@ -348,11 +348,30 @@
}
}
+/* If a TBF is attached to an MS, it is either in ms->{dl,ul}_tbf or in ms->old_tbfs list */
+static bool ms_tbf_is_attached(const struct GprsMs *ms, const struct gprs_rlcmac_tbf *tbf)
+{
+ const struct llist_item *pos;
+ OSMO_ASSERT(ms);
+ OSMO_ASSERT(tbf);
+ OSMO_ASSERT(tbf_ms(tbf) == ms);
+
+ if (tbf == ul_tbf_as_tbf_const(ms->ul_tbf))
+ return true;
+
+ if (tbf == dl_tbf_as_tbf_const(ms->dl_tbf))
+ return true;
+
+ llist_for_each_entry(pos, &ms->old_tbfs, list) {
+ const struct gprs_rlcmac_tbf *tmp_tbf = (struct gprs_rlcmac_tbf *)pos->entry;
+ if (tmp_tbf == tbf)
+ return true;
+ }
+ return false;
+}
+
static void ms_attach_ul_tbf(struct GprsMs *ms, struct gprs_rlcmac_ul_tbf *tbf)
{
- if (ms->ul_tbf == tbf)
- return;
-
LOGPMS(ms, DRLCMAC, LOGL_INFO, "Attaching UL TBF: %s\n", tbf_name((struct gprs_rlcmac_tbf *)tbf));
ms_ref(ms, __func__);
@@ -369,9 +388,6 @@
static void ms_attach_dl_tbf(struct GprsMs *ms, struct gprs_rlcmac_dl_tbf *tbf)
{
- if (ms->dl_tbf == tbf)
- return;
-
LOGPMS(ms, DRLCMAC, LOGL_INFO, "Attaching DL TBF: %s\n", tbf_name((struct gprs_rlcmac_tbf *)tbf));
ms_ref(ms, __func__);
@@ -390,6 +406,7 @@
{
OSMO_ASSERT(ms);
OSMO_ASSERT(tbf);
+ OSMO_ASSERT(!ms_tbf_is_attached(ms, tbf));
if (tbf_direction(tbf) == GPRS_RLCMAC_DL_TBF)
ms_attach_dl_tbf(ms, tbf_as_dl_tbf(tbf));
@@ -399,34 +416,26 @@
void ms_detach_tbf(struct GprsMs *ms, struct gprs_rlcmac_tbf *tbf)
{
- if (tbf == (struct gprs_rlcmac_tbf *)(ms->ul_tbf)) {
+ OSMO_ASSERT(tbf_ms(tbf) == ms);
+
+ /* In general this should not happen, but it can happen if during TBF
+ * allocation something fails before tbf->setup() called ms_attach_tbf(). */
+ if (!ms_tbf_is_attached(ms, tbf))
+ return;
+
+ if (tbf == ul_tbf_as_tbf(ms->ul_tbf)) {
ms->ul_tbf = NULL;
- } else if (tbf == (struct gprs_rlcmac_tbf *)(ms->dl_tbf)) {
+ } else if (tbf == dl_tbf_as_tbf(ms->dl_tbf)) {
ms->dl_tbf = NULL;
} else {
- bool found = false;
-
- struct llist_item *pos, *tmp;
- llist_for_each_entry_safe(pos, tmp, &ms->old_tbfs, list) {
- struct gprs_rlcmac_tbf *tmp_tbf = (struct gprs_rlcmac_tbf *)pos->entry;
- if (tmp_tbf == tbf) {
- llist_del(&pos->list);
- found = true;
- break;
- }
- }
-
- /* Protect against recursive calls via set_ms() */
- if (!found)
- return;
+ /* We know from ms_tbf_is_attached()==true check above that tbf
+ * is in ms->old_tbfs, no need to look it up again. */
+ llist_del(tbf_ms_list(tbf));
}
LOGPMS(ms, DRLCMAC, LOGL_INFO, "Detaching TBF: %s\n",
tbf_name(tbf));
- if (tbf_ms(tbf) == ms)
- tbf_set_ms(tbf, NULL);
-
if (!ms->dl_tbf && !ms->ul_tbf) {
ms_set_reserved_slots(ms, NULL, 0, 0);
ms->first_common_ts = NULL;
diff --git a/tests/ms/MsTest.cpp b/tests/ms/MsTest.cpp
index a8febd0..40e8381 100644
--- a/tests/ms/MsTest.cpp
+++ b/tests/ms/MsTest.cpp
@@ -443,7 +443,7 @@
OSMO_ASSERT(ms != NULL);
ul_tbf = alloc_ul_tbf(bts, ms);
ms_attach_tbf(ms, ul_tbf);
- ms_detach_tbf(ms, ul_tbf);
+ tbf_set_ms(ul_tbf, NULL);
ms = bts_get_ms_by_tlli(bts, tlli + 0, GSM_RESERVED_TMSI);
OSMO_ASSERT(ms == NULL);
ms = bts_get_ms_by_tlli(bts, tlli + 1, GSM_RESERVED_TMSI);
@@ -452,8 +452,8 @@
/* delete ms */
ms = bts_get_ms_by_tlli(bts, tlli + 1, GSM_RESERVED_TMSI);
OSMO_ASSERT(ms != NULL);
- ms_attach_tbf(ms, ul_tbf);
- ms_detach_tbf(ms, ul_tbf);
+ tbf_set_ms(ul_tbf, ms);
+ tbf_set_ms(ul_tbf, NULL);
ms = bts_get_ms_by_tlli(bts, tlli + 1, GSM_RESERVED_TMSI);
OSMO_ASSERT(ms == NULL);
diff --git a/tests/ms/MsTest.err b/tests/ms/MsTest.err
index 3d271ae..3f2f520 100644
--- a/tests/ms/MsTest.err
+++ b/tests/ms/MsTest.err
@@ -63,10 +63,10 @@
MS(IMSI-001001987654321:TLLI-0xffeeddbb:TA-220:MSCLS-0-0:UL): - ms_attach_ul_tbf: now used by 0 (-)
MS(IMSI-001001987654321:TLLI-0xffeeddbb:TA-220:MSCLS-0-0) Detaching TBF: TBF(UL:STATE-NEW:GPRS:IMSI-001001987654321:TLLI-0xffeeddbb)
MS(IMSI-001001987654321:TLLI-0xffeeddbb:TA-220:MSCLS-0-0) Destroying MS object
-MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0) Attaching UL TBF: TBF(UL:STATE-NEW:GPRS)
+MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0) Attaching UL TBF: TBF(UL:STATE-NEW:GPRS:IMSI-001001987654322:TLLI-0xffeeddbc)
MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0): + ms_attach_ul_tbf: now used by 1 (ms_attach_ul_tbf)
MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0:UL): - ms_attach_ul_tbf: now used by 0 (-)
-MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0) Detaching TBF: TBF(UL:STATE-NEW:GPRS)
+MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0) Detaching TBF: TBF(UL:STATE-NEW:GPRS:IMSI-001001987654322:TLLI-0xffeeddbc)
MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0) Destroying MS object
Creating MS object
Modifying MS object, UL TLLI: 0xffffffff -> 0xffeeddbb, not yet confirmed