diff mbox

[osmo-pcu,4/5] bts, tbf: Change the TBF return type of functions to the ul/dl version

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

Commit Message

Daniel Willmann July 16, 2014, 5:04 p.m. UTC 
Many functions only ever deal with or return a UL or a DL TBF.
Explicitly change the type to reflect which TBF is used where.

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

Comments

Holger Freyther July 17, 2014, 6:52 a.m. UTC | #1 
On Wed, Jul 16, 2014 at 07:04:31PM +0200, Daniel Willmann wrote:
>  void gprs_rlcmac_pdch::rcv_resource_request(Packet_Resource_Request_t *request, uint32_t fn)
>  {
> -	struct gprs_rlcmac_tbf *tbf;
> +	struct gprs_rlcmac_ul_tbf *ul_tbf;
> +	struct gprs_rlcmac_dl_tbf *dl_tbf;

Limit the dl_tbf (and ul_tbf) in scope. The dl_tbf one part of the code
is dealing with is not the one the other is using. We should not accidently
leak a tbf into another scope just because this method is huge.

In general cppcheck's style warnings complain about this kind of thing
Holger Freyther July 17, 2014, 6:55 a.m. UTC | #2 
On Wed, Jul 16, 2014 at 07:04:31PM +0200, Daniel Willmann wrote:
> -	LOGP(DRLCMAC, LOGL_ERROR, "RX: [PCU <- BTS] %s FIXME: Packet resource request\n", tbf_name(tbf));
> +	LOGP(DRLCMAC, LOGL_ERROR, "RX: [PCU <- BTS] %s FIXME: Packet resource request\n", tbf_name(ul_tbf));

And the compiler just re-inforced me with a warning:

home/ich/install/openbsc/include/osmocom/core/logging.h: In member function 'void gprs_rlcmac_pdch::rcv_resource_request(Packet_Resource_Request_t*, uint32_t)':
/home/ich/install/openbsc/include/osmocom/core/logging.h:42:53: warning: 'ul_tbf' may be used uninitialized in this function [-Wmaybe-uninitialized]
  logp2(ss, level, __FILE__, __LINE__, 0, fmt, ##args)


After the last if/else either dl_tbf or ul_tbf will be assigned. Maybe just
move the warning into the if/else and remove the declaration of the dl_tbf
ul_tbf.
diff mbox

Patch

diff --git a/src/bts.cpp b/src/bts.cpp
index 68d72f2..8489431 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -417,7 +417,7 @@  int BTS::rcv_imm_ass_cnf(const uint8_t *data, uint32_t fn)
 
 int BTS::rcv_rach(uint8_t ra, uint32_t Fn, int16_t qta)
 {
-	struct gprs_rlcmac_tbf *tbf;
+	struct gprs_rlcmac_ul_tbf *tbf;
 	uint8_t trx_no, ts_no = 0;
 	int8_t tfi; /* must be signed */
 	uint8_t sb = 0;
@@ -461,7 +461,7 @@  int BTS::rcv_rach(uint8_t ra, uint32_t Fn, int16_t qta)
 			return -EBUSY;
 		}
 		/* set class to 0, since we don't know the multislot class yet */
-		tbf = tbf_alloc(&m_bts, NULL, GPRS_RLCMAC_UL_TBF, tfi, trx_no, 0, 1);
+		tbf = (gprs_rlcmac_ul_tbf *)tbf_alloc(&m_bts, NULL, GPRS_RLCMAC_UL_TBF, tfi, trx_no, 0, 1);
 		if (!tbf) {
 			LOGP(DRLCMAC, LOGL_NOTICE, "No PDCH resource\n");
 			/* FIXME: send reject */
@@ -692,7 +692,7 @@  void gprs_rlcmac_pdch::add_paging(struct gprs_rlcmac_paging *pag)
  */
 int gprs_rlcmac_pdch::rcv_data_block_acknowledged(uint8_t *data, uint8_t len, int8_t rssi)
 {
-	struct gprs_rlcmac_tbf *tbf;
+	struct gprs_rlcmac_ul_tbf *tbf;
 	struct rlc_ul_header *rh = (struct rlc_ul_header *)data;
 
 	switch (len) {
@@ -867,24 +867,24 @@  void gprs_rlcmac_pdch::rcv_control_dl_ack_nack(Packet_Downlink_Ack_Nack_t *ack_n
 
 void gprs_rlcmac_pdch::rcv_resource_request(Packet_Resource_Request_t *request, uint32_t fn)
 {
-	struct gprs_rlcmac_tbf *tbf;
+	struct gprs_rlcmac_ul_tbf *ul_tbf;
+	struct gprs_rlcmac_dl_tbf *dl_tbf;
 	struct gprs_rlcmac_sba *sba;
 	int rc;
 
 	if (request->ID.UnionType) {
 		uint32_t tlli = request->ID.u.TLLI;
 		uint8_t ms_class = 0;
-		struct gprs_rlcmac_tbf *dl_tbf;
 		uint8_t ta;
 
-		tbf = bts()->ul_tbf_by_tlli(tlli);
-		if (tbf) {
+		ul_tbf = bts()->ul_tbf_by_tlli(tlli);
+		if (ul_tbf) {
 			LOGP(DRLCMACUL, LOGL_NOTICE, "Got RACH from "
 				"TLLI=0x%08x while %s still "
 				"exists. Killing pending DL TBF\n",
-				tlli, tbf_name(tbf));
-			tbf_free(tbf);
-			tbf = NULL;
+				tlli, tbf_name(ul_tbf));
+			tbf_free(ul_tbf);
+			ul_tbf = NULL;
 		}
 
 		if ((dl_tbf = bts()->dl_tbf_by_tlli(tlli))) {
@@ -918,34 +918,34 @@  void gprs_rlcmac_pdch::rcv_resource_request(Packet_Resource_Request_t *request,
 			ms_class = Decoding::get_ms_class_by_capability(&request->MS_Radio_Access_capability);
 		if (!ms_class)
 			LOGP(DRLCMAC, LOGL_NOTICE, "MS does not give us a class.\n");
-		tbf = tbf_alloc_ul(bts_data(), trx_no(), ms_class, tlli, ta, NULL);
-		if (!tbf)
+		ul_tbf = tbf_alloc_ul(bts_data(), trx_no(), ms_class, tlli, ta, NULL);
+		if (!ul_tbf)
 			return;
 		/* set control ts to current MS's TS, until assignment complete */
 		LOGP(DRLCMAC, LOGL_DEBUG, "Change control TS to %d until assinment is complete.\n", ts_no);
-		tbf->control_ts = ts_no;
+		ul_tbf->control_ts = ts_no;
 		/* schedule uplink assignment */
-		tbf->ul_ass_state = GPRS_RLCMAC_UL_ASS_SEND_ASS;
+		ul_tbf->ul_ass_state = GPRS_RLCMAC_UL_ASS_SEND_ASS;
 		return;
 	}
 
 	if (request->ID.u.Global_TFI.UnionType) {
 		int8_t tfi = request->ID.u.Global_TFI.u.DOWNLINK_TFI;
-		tbf = bts()->dl_tbf_by_tfi(tfi, trx_no());
-		if (!tbf) {
+		dl_tbf = bts()->dl_tbf_by_tfi(tfi, trx_no());
+		if (!dl_tbf) {
 			LOGP(DRLCMAC, LOGL_NOTICE, "PACKET RESSOURCE REQ unknown downlink TFI=%d\n", tfi);
 			return;
 		}
 	} else {
 		int8_t tfi = request->ID.u.Global_TFI.u.UPLINK_TFI;
-		tbf = bts()->ul_tbf_by_tfi(tfi, trx_no());
-		if (!tbf) {
+		ul_tbf = bts()->ul_tbf_by_tfi(tfi, trx_no());
+		if (!ul_tbf) {
 			LOGP(DRLCMAC, LOGL_NOTICE, "PACKET RESSOURCE REQ unknown uplink TFI=%d\n", tfi);
 			return;
 		}
 	}
 
-	LOGP(DRLCMAC, LOGL_ERROR, "RX: [PCU <- BTS] %s FIXME: Packet resource request\n", tbf_name(tbf));
+	LOGP(DRLCMAC, LOGL_ERROR, "RX: [PCU <- BTS] %s FIXME: Packet resource request\n", tbf_name(ul_tbf));
 }
 
 void gprs_rlcmac_pdch::rcv_measurement_report(Packet_Measurement_Report_t *report, uint32_t fn)
diff --git a/src/tbf.cpp b/src/tbf.cpp
index 39549f1..15301a1 100644
--- a/src/tbf.cpp
+++ b/src/tbf.cpp
@@ -214,12 +214,12 @@  int gprs_rlcmac_tbf::handle(struct gprs_rlcmac_bts *bts,
 	return tbf_new_dl_assignment(bts, imsi, tlli, ms_class, data, len);
 }
 
-struct gprs_rlcmac_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts,
+gprs_rlcmac_ul_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts,
 	int8_t use_trx, uint8_t ms_class,
 	uint32_t tlli, uint8_t ta, struct gprs_rlcmac_tbf *dl_tbf)
 {
 	uint8_t trx;
-	struct gprs_rlcmac_tbf *tbf;
+	struct gprs_rlcmac_ul_tbf *tbf;
 	int8_t tfi; /* must be signed */
 
 #warning "Copy and paste with tbf_new_dl_assignment"
@@ -231,7 +231,7 @@  struct gprs_rlcmac_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts,
 		return NULL;
 	}
 	/* use multislot class of downlink TBF */
-	tbf = tbf_alloc(bts, dl_tbf, GPRS_RLCMAC_UL_TBF, tfi, trx, ms_class, 0);
+	tbf = (gprs_rlcmac_ul_tbf *)tbf_alloc(bts, dl_tbf, GPRS_RLCMAC_UL_TBF, tfi, trx, ms_class, 0);
 	if (!tbf) {
 		LOGP(DRLCMAC, LOGL_NOTICE, "No PDCH resource\n");
 		/* FIXME: send reject */
diff --git a/src/tbf.h b/src/tbf.h
index 8464e19..d650021 100644
--- a/src/tbf.h
+++ b/src/tbf.h
@@ -259,7 +259,7 @@  protected:
 };
 
 
-struct gprs_rlcmac_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts,
+struct gprs_rlcmac_ul_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts,
 	int8_t use_trx, uint8_t ms_class,
 	uint32_t tlli, uint8_t ta, struct gprs_rlcmac_tbf *dl_tbf);