diff mbox

[osmo-pcu,1/5] bts: Ensure tbf direction with OSMO_ASSERT()

Message ID 2207c5e4ef0565ab782196a80cc45dcc458938e4.1405530272.git.daniel@totalueberwachung.de
State Accepted
Headers show

Commit Message

Daniel Willmann July 16, 2014, 5:04 p.m. UTC 
In the lookup functions make sure that we are actually returning tbfs
with the expected direction.

Ticket: SYS#389
Sponsored-by: On-Waves ehf
---
 src/bts.cpp | 22 ++++++++++++++++------
 src/bts.h   |  3 ++-
 2 files changed, 18 insertions(+), 7 deletions(-)

Comments

Holger Freyther July 17, 2014, 6:46 a.m. UTC | #1 
On Wed, Jul 16, 2014 at 07:04:28PM +0200, Daniel Willmann wrote:

Hi!

as we have to move forward I will merge it. One comment in regard to
the approach taken here.

> +gprs_rlcmac_tbf *gprs_rlcmac_pdch::tbf_from_list_by_tfi(struct llist_head *tbf_list, uint8_t tfi,
> +		enum gprs_rlcmac_tbf_direction dir)
>  {
>  	gprs_rlcmac_tbf *tbf;
>  
>  	llist_for_each_entry(tbf, tbf_list, list) {
> +		OSMO_ASSERT(tbf->direction == dir);

> +	return tbf_from_list_by_tfi(&bts_data()->ul_tbfs, tfi, GPRS_RLCMAC_UL_TBF);
> +	return tbf_from_list_by_tfi(&bts_data()->dl_tbfs, tfi, GPRS_RLCMAC_DL_TBF);

assert it on _entry_ into the ul_tbfs and dl_tbfs. If we only have one
place where the list is manipulated we can easily pay the price on entry.
Daniel Willmann Aug. 7, 2014, 2:20 p.m. UTC | #2 
Hi,
On Thu, 2014-07-17 at 08:46, Holger Hans Peter Freyther wrote:
> On Wed, Jul 16, 2014 at 07:04:28PM +0200, Daniel Willmann wrote:
> as we have to move forward I will merge it. One comment in regard to
> the approach taken here.
> 
> > +gprs_rlcmac_tbf *gprs_rlcmac_pdch::tbf_from_list_by_tfi(struct llist_head *tbf_list, uint8_t tfi,
> > +		enum gprs_rlcmac_tbf_direction dir)
> >  {
> >  	gprs_rlcmac_tbf *tbf;
> >  
> >  	llist_for_each_entry(tbf, tbf_list, list) {
> > +		OSMO_ASSERT(tbf->direction == dir);
> 
> > +	return tbf_from_list_by_tfi(&bts_data()->ul_tbfs, tfi, GPRS_RLCMAC_UL_TBF);
> > +	return tbf_from_list_by_tfi(&bts_data()->dl_tbfs, tfi, GPRS_RLCMAC_DL_TBF);
> 
> assert it on _entry_ into the ul_tbfs and dl_tbfs. If we only have one
> place where the list is manipulated we can easily pay the price on entry.

This is obsolete now anyway as the functions that do llist_add
explicitly allocate a DL/UL TBF.

I have a patch that will remove the ASSERTs completely

Regards,
Daniel Willmann
diff mbox

Patch

diff --git a/src/bts.cpp b/src/bts.cpp
index 08baee0..f614dab 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -234,12 +234,14 @@  gprs_rlcmac_tbf *BTS::tbf_by_tlli(uint32_t tlli, enum gprs_rlcmac_tbf_direction
 	struct gprs_rlcmac_tbf *tbf;
 	if (dir == GPRS_RLCMAC_UL_TBF) {
 		llist_for_each_entry(tbf, &m_bts.ul_tbfs, list) {
+			OSMO_ASSERT(tbf->direction == dir);
 			if (tbf->state_is_not(GPRS_RLCMAC_RELEASING)
 			 && tbf->tlli() == tlli && tbf->is_tlli_valid())
 				return tbf;
 		}
 	} else {
 		llist_for_each_entry(tbf, &m_bts.dl_tbfs, list) {
+			OSMO_ASSERT(tbf->direction == dir);
 			if (tbf->state_is_not(GPRS_RLCMAC_RELEASING)
 			 && tbf->tlli() == tlli)
 				return tbf;
@@ -258,8 +260,10 @@  gprs_rlcmac_tbf *BTS::dl_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts)
 		if (tbf->state_is_not(GPRS_RLCMAC_RELEASING)
 		 && tbf->poll_state == GPRS_RLCMAC_POLL_SCHED
 		 && tbf->poll_fn == fn && tbf->trx->trx_no == trx
-		 && tbf->control_ts == ts)
+		 && tbf->control_ts == ts) {
+			OSMO_ASSERT(tbf->direction == GPRS_RLCMAC_DL_TBF);
 			return tbf;
+		}
 	}
 	return NULL;
 }
@@ -273,8 +277,10 @@  gprs_rlcmac_tbf *BTS::ul_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts)
 		if (tbf->state_is_not(GPRS_RLCMAC_RELEASING)
 		 && tbf->poll_state == GPRS_RLCMAC_POLL_SCHED
 		 && tbf->poll_fn == fn && tbf->trx->trx_no == trx
-		 && tbf->control_ts == ts)
+		 && tbf->control_ts == ts) {
+			OSMO_ASSERT(tbf->direction == GPRS_RLCMAC_UL_TBF);
 			return tbf;
+		}
 	}
 	return NULL;
 }
@@ -307,8 +313,10 @@  gprs_rlcmac_tbf *BTS::tbf_by_tfi(uint8_t tfi, uint8_t trx,
 	if (!tbf)
 		return NULL;
 
-	if (tbf->state_is_not(GPRS_RLCMAC_RELEASING))
+	if (tbf->state_is_not(GPRS_RLCMAC_RELEASING)) {
+		OSMO_ASSERT(tbf->direction == dir);
 		return tbf;
+	}
 
 	return NULL;
 }
@@ -1023,11 +1031,13 @@  int gprs_rlcmac_pdch::rcv_block(uint8_t *data, uint8_t len, uint32_t fn, int8_t
 	return rc;
 }
 
-gprs_rlcmac_tbf *gprs_rlcmac_pdch::tbf_from_list_by_tfi(struct llist_head *tbf_list, uint8_t tfi)
+gprs_rlcmac_tbf *gprs_rlcmac_pdch::tbf_from_list_by_tfi(struct llist_head *tbf_list, uint8_t tfi,
+		enum gprs_rlcmac_tbf_direction dir)
 {
 	gprs_rlcmac_tbf *tbf;
 
 	llist_for_each_entry(tbf, tbf_list, list) {
+		OSMO_ASSERT(tbf->direction == dir);
 		if (tbf->tfi() != tfi)
 			continue;
 		if (!tbf->pdch[ts_no])
@@ -1039,10 +1049,10 @@  gprs_rlcmac_tbf *gprs_rlcmac_pdch::tbf_from_list_by_tfi(struct llist_head *tbf_l
 
 gprs_rlcmac_tbf *gprs_rlcmac_pdch::ul_tbf_by_tfi(uint8_t tfi)
 {
-	return tbf_from_list_by_tfi(&bts_data()->ul_tbfs, tfi);
+	return tbf_from_list_by_tfi(&bts_data()->ul_tbfs, tfi, GPRS_RLCMAC_UL_TBF);
 }
 
 gprs_rlcmac_tbf *gprs_rlcmac_pdch::dl_tbf_by_tfi(uint8_t tfi)
 {
-	return tbf_from_list_by_tfi(&bts_data()->dl_tbfs, tfi);
+	return tbf_from_list_by_tfi(&bts_data()->dl_tbfs, tfi, GPRS_RLCMAC_DL_TBF);
 }
diff --git a/src/bts.h b/src/bts.h
index 621bcba..a0b808e 100644
--- a/src/bts.h
+++ b/src/bts.h
@@ -87,7 +87,8 @@  private:
 	void rcv_control_dl_ack_nack(Packet_Downlink_Ack_Nack_t *, uint32_t fn);
 	void rcv_resource_request(Packet_Resource_Request_t *t, uint32_t fn);
 	void rcv_measurement_report(Packet_Measurement_Report_t *t, uint32_t fn);
-	gprs_rlcmac_tbf *tbf_from_list_by_tfi(struct llist_head *tbf_list, uint8_t tfi);
+	gprs_rlcmac_tbf *tbf_from_list_by_tfi(struct llist_head *tbf_list, uint8_t tfi,
+		enum gprs_rlcmac_tbf_direction dir);
 #endif
 };