cellular-infrastructure
/
osmo-pcu
mirror of https://gerrit.osmocom.org/osmo-pcu
9
0
Fork 1
Code Issues Releases Wiki Activity

tbf: Clean up gprs_rlcmac_dl_tbf::handle() 

Avoid passing tons of params to internal helper function
tbf_nel_dl_assignment() in order to either fetch again the ms object or
create a new one. Let's instead create the ms function earlier if needed
and fill it with all the discovered information prior to calling the
helper function. This provides cleaner code and also better log output.

This way we also avoid trying to fill the MS twice and unneeded
getter+setter for TA.

tbf::imsi(): There' always an ms, so simply forward call to
ms()->imsi().

We can also get rid of assign_imsi, since the modified code is the only
place where it's used and there's already some code in place there to
update the MS. We instead merge it with set_imsi and keep the
duplication code to catch possible bugs from callers.

Move merge_and_clear_ms from tbf class to GprsMS, where it really
belongs.

Change-Id: Id18098bac3cff26fc4a8d2f419e21641a1f4c83b
This commit is contained in:
Pau Espin 2020-10-26 12:40:11 +01:00 committed by pespin
parent 87c6dd3c26
commit 528820dbe1
7 changed files with 283 additions and 334 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
src/gprs_ms.cpp
View File

@ -377,11 +377,10 @@ void GprsMs::reset()
void GprsMs::merge_old_ms(GprsMs *old_ms)
{
if (old_ms == this)
return;
OSMO_ASSERT(old_ms != this);
if (strlen(imsi()) == 0 && strlen(old_ms->imsi()) != 0)
set_imsi(old_ms->imsi());
osmo_strlcpy(m_imsi, old_ms->imsi(), sizeof(m_imsi));
if (!ms_class() && old_ms->ms_class())
set_ms_class(old_ms->ms_class());
@ -394,6 +393,22 @@ void GprsMs::merge_old_ms(GprsMs *old_ms)
old_ms->reset();
}
void GprsMs::merge_and_clear_ms(GprsMs *old_ms)
{
OSMO_ASSERT(old_ms != this);
GprsMs::Guard guard_old(old_ms);
/* Clean up the old MS object */
/* TODO: Use timer? */
if (old_ms->ul_tbf() && !old_ms->ul_tbf()->timers_pending(T_MAX))
tbf_free(old_ms->ul_tbf());
if (old_ms->dl_tbf() && !old_ms->dl_tbf()->timers_pending(T_MAX))
tbf_free(old_ms->dl_tbf());
merge_old_ms(old_ms);
}
void GprsMs::set_tlli(uint32_t tlli)
{
if (tlli == m_tlli || tlli == m_new_ul_tlli)
@ -466,6 +481,24 @@ void GprsMs::set_imsi(const char *imsi)
"Modifying MS object, TLLI = 0x%08x, IMSI '%s' -> '%s'\n",
tlli(), m_imsi, imsi);
GprsMs *old_ms = m_bts->ms_store().get_ms(0, 0, imsi);
/* Check if we are going to store a different MS object with already
existing IMSI. This is probably a bug in code calling this function,
since it should take care of this explicitly */
if (old_ms) {
/* We cannot find m_ms by IMSI since we know that it has a
* different IMSI */
OSMO_ASSERT(old_ms != this);
LOGPMS(this, DRLCMAC, LOGL_NOTICE,
"IMSI '%s' was already assigned to another "
"MS object: TLLI = 0x%08x, that IMSI will be removed\n",
imsi, old_ms->tlli());
merge_and_clear_ms(old_ms);
}
osmo_strlcpy(m_imsi, imsi, sizeof(m_imsi));
}

3
src/gprs_ms.h
View File

@ -68,7 +68,7 @@ public:
void set_callback(Callback *cb) {m_cb = cb;}
void merge_old_ms(GprsMs *old_ms);
void merge_and_clear_ms(GprsMs *old_ms);
gprs_rlcmac_ul_tbf *ul_tbf() const {return m_ul_tbf;}
gprs_rlcmac_dl_tbf *dl_tbf() const {return m_dl_tbf;}
@ -144,6 +144,7 @@ public:
bool app_info_pending;
protected:
void merge_old_ms(GprsMs *old_ms);
void update_status();
GprsMs *ref();
void unref();

70
src/tbf.cpp
View File

@ -188,40 +188,7 @@ uint32_t gprs_rlcmac_tbf::tlli() const
const char *gprs_rlcmac_tbf::imsi() const
{
static const char nullc = 0;
return m_ms ? m_ms->imsi() : &nullc;
}
void gprs_rlcmac_tbf::assign_imsi(const char *imsi_)
{
GprsMs *old_ms;
if (!imsi_ || !m_ms) {
LOGPTBF(this, LOGL_ERROR,
"failed to assign IMSI: missing IMSI or MS object\n");
return;
}
if (strcmp(imsi_, imsi()) == 0)
return;
/* really change the IMSI */
old_ms = bts->ms_store().get_ms(0, 0, imsi_);
if (old_ms) {
/* We cannot find m_ms by IMSI since we know that it has a
* different IMSI */
OSMO_ASSERT(old_ms != m_ms);
LOGPTBF(this, LOGL_INFO,
"IMSI '%s' was already assigned to another "
"MS object: TLLI = 0x%08x, that IMSI will be removed\n",
imsi_, old_ms->tlli());
merge_and_clear_ms(old_ms);
}
m_ms->set_imsi(imsi_);
return m_ms->imsi();
}
uint8_t gprs_rlcmac_tbf::ta() const
@ -283,37 +250,6 @@ void gprs_rlcmac_tbf::set_ms(GprsMs *ms)
m_ms->attach_tbf(this);
}
void gprs_rlcmac_tbf::merge_and_clear_ms(GprsMs *old_ms)
{
if (old_ms == ms())
return;
GprsMs::Guard guard_old(old_ms);
/* Clean up the old MS object */
/* TODO: Use timer? */
if (old_ms->ul_tbf() && !old_ms->ul_tbf()->timers_pending(T_MAX)) {
if (old_ms->ul_tbf() == this) {
LOGPTBF(this, LOGL_ERROR,
"is referred by the old MS and will not be deleted\n");
set_ms(NULL);
} else {
tbf_free(old_ms->ul_tbf());
}
}
if (old_ms->dl_tbf() && !old_ms->dl_tbf()->timers_pending(T_MAX)) {
if (old_ms->dl_tbf() == this) {
LOGPTBF(this, LOGL_ERROR,
"is referred by the old MS and will not be deleted\n");
set_ms(NULL);
} else {
tbf_free(old_ms->dl_tbf());
}
}
ms()->merge_old_ms(old_ms);
}
void gprs_rlcmac_tbf::update_ms(uint32_t tlli, enum gprs_rlcmac_tbf_direction dir)
{
if (!tlli)
@ -322,13 +258,13 @@ void gprs_rlcmac_tbf::update_ms(uint32_t tlli, enum gprs_rlcmac_tbf_direction di
/* TODO: When the TLLI does not match the ms, check if there is another
* MS object that belongs to that TLLI and if yes make sure one of them
* gets deleted. This is the same problem that can arise with
* assign_imsi() so there should be a unified solution */
* IMSI in gprs_rlcmac_dl_tbf::handle() so there should be a unified solution */
if (!ms()->check_tlli(tlli)) {
GprsMs *old_ms;
old_ms = bts->ms_store().get_ms(tlli, 0, NULL);
if (old_ms)
merge_and_clear_ms(old_ms);
ms()->merge_and_clear_ms(old_ms);
}
if (dir == GPRS_RLCMAC_UL_TBF)

1
src/tbf.h
View File

@ -250,7 +250,6 @@ struct gprs_rlcmac_tbf {
bool is_tfi_assigned() const;
const char *imsi() const;
void assign_imsi(const char *imsi);
uint8_t ta() const;
void set_ta(uint8_t);
uint8_t ms_class() const;

40
src/tbf_dl.cpp
View File

@ -251,27 +251,15 @@ int gprs_rlcmac_dl_tbf::append_data(const uint8_t ms_class,
return 0;
}
static int tbf_new_dl_assignment(struct gprs_rlcmac_bts *bts,
const char *imsi,
const uint32_t tlli, const uint32_t tlli_old,
const uint8_t ms_class,
const uint8_t egprs_ms_class,
struct gprs_rlcmac_dl_tbf **tbf)
static int tbf_new_dl_assignment(struct gprs_rlcmac_bts *bts, GprsMs *ms,
struct gprs_rlcmac_dl_tbf **tbf)
{
bool ss;
int8_t use_trx;
uint8_t ta = GSM48_TA_INVALID;
struct gprs_rlcmac_ul_tbf *ul_tbf = NULL, *old_ul_tbf;
struct gprs_rlcmac_dl_tbf *dl_tbf = NULL;
GprsMs *ms;
/* check for uplink data, so we copy our informations */
ms = bts->bts->ms_store().get_ms(tlli, tlli_old, imsi);
if (!ms)
ms = bts->bts->ms_alloc(ms_class, egprs_ms_class); /* ms class updated later */
ul_tbf = ms->ul_tbf();
ta = ms->ta();
if (ul_tbf && ul_tbf->m_contention_resolution_done
&& !ul_tbf->m_final_ack_sent) {
@ -293,14 +281,9 @@ static int tbf_new_dl_assignment(struct gprs_rlcmac_bts *bts,
LOGP(DTBF, LOGL_NOTICE, "No PDCH resource\n");
return -EBUSY;
}
dl_tbf->update_ms(tlli, GPRS_RLCMAC_DL_TBF);
dl_tbf->ms()->set_ta(ta);
LOGPTBFDL(dl_tbf, LOGL_DEBUG, "[DOWNLINK] START\n");
/* Store IMSI for later look-up and PCH retransmission */
dl_tbf->assign_imsi(imsi);
/* trigger downlink assignment and set state to ASSIGN.
* we don't use old_downlink, so the possible uplink is used
* to trigger downlink assignment. if there is no uplink,
@ -354,28 +337,23 @@ int gprs_rlcmac_dl_tbf::handle(struct gprs_rlcmac_bts *bts,
/* Move the DL TBF to the new MS */
dl_tbf->set_ms(ms);
}
/* Clean up the old MS object */
/* TODO: Put this into a separate function, use timer? */
if (ms_old->ul_tbf() && !ms_old->ul_tbf()->timers_pending(T_MAX))
tbf_free(ms_old->ul_tbf());
if (ms_old->dl_tbf() && !ms_old->dl_tbf()->timers_pending(T_MAX))
tbf_free(ms_old->dl_tbf());
ms->merge_old_ms(ms_old);
ms->merge_and_clear_ms(ms_old);
}
}
if (!ms)
ms = bts->bts->ms_alloc(ms_class, egprs_ms_class);
ms->set_imsi(imsi);
ms->confirm_tlli(tlli);
if (!dl_tbf) {
rc = tbf_new_dl_assignment(bts, imsi, tlli, tlli_old,
ms_class, egprs_ms_class, &dl_tbf);
rc = tbf_new_dl_assignment(bts, ms, &dl_tbf);
if (rc < 0)
return rc;
}
/* TODO: ms_class vs. egprs_ms_class is not handled here */
rc = dl_tbf->append_data(ms_class, delay_csec, data, len);
dl_tbf->update_ms(tlli, GPRS_RLCMAC_DL_TBF);
dl_tbf->assign_imsi(imsi);
return rc;
}

8
tests/tbf/TbfTest.cpp
View File

@ -420,7 +420,7 @@ static void test_tbf_imsi()
dl_tbf[0]->update_ms(0xf1000001, GPRS_RLCMAC_DL_TBF);
dl_tbf[1]->update_ms(0xf1000002, GPRS_RLCMAC_DL_TBF);
dl_tbf[0]->assign_imsi("001001000000001");
dl_tbf[0]->ms()->set_imsi("001001000000001");
ms1 = the_bts.ms_store().get_ms(0, 0, "001001000000001");
OSMO_ASSERT(ms1 != NULL);
ms2 = the_bts.ms_store().get_ms(0xf1000001);
@ -429,7 +429,7 @@ static void test_tbf_imsi()
OSMO_ASSERT(ms1 == ms2);
/* change the IMSI on TBF 0 */
dl_tbf[0]->assign_imsi("001001000000002");
dl_tbf[0]->ms()->set_imsi("001001000000002");
ms1 = the_bts.ms_store().get_ms(0, 0, "001001000000001");
OSMO_ASSERT(ms1 == NULL);
ms1 = the_bts.ms_store().get_ms(0, 0, "001001000000002");
@ -437,10 +437,10 @@ static void test_tbf_imsi()
OSMO_ASSERT(strcmp(ms2->imsi(), "001001000000002") == 0);
OSMO_ASSERT(ms1 == ms2);
/* use the same IMSI on TBF 2 */
/* use the same IMSI on TBF 1 */
{
GprsMs::Guard guard(ms2);
dl_tbf[1]->assign_imsi("001001000000002");
dl_tbf[1]->ms()->set_imsi("001001000000002");
ms1 = the_bts.ms_store().get_ms(0, 0, "001001000000002");
OSMO_ASSERT(ms1 != NULL);
OSMO_ASSERT(ms1 != ms2);

456
tests/tbf/TbfTest.err
View File

File diff suppressed because it is too large Load Diff