[osmo-pcu 1/5] bts: Ensure tbf direction with OSMO_ASSERT()
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(-) 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 }; -- 1.8.4.2
Ticket: SYS#389 Sponsored-by: On-Waves ehf --- src/tbf.cpp | 6 +++++- src/tbf.h | 6 ++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/tbf.cpp b/src/tbf.cpp index f913f97..39549f1 100644 --- a/src/tbf.cpp +++ b/src/tbf.cpp @@ -495,7 +495,11 @@ struct gprs_rlcmac_tbf *tbf_alloc(struct gprs_rlcmac_bts *bts, if (trx >= 8 || tfi >= 32) return NULL; - tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_tbf); + if (dir == GPRS_RLCMAC_UL_TBF) + tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_ul_tbf); + else + tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_dl_tbf); + if (!tbf) return NULL; diff --git a/src/tbf.h b/src/tbf.h index 80e2068..8464e19 100644 --- a/src/tbf.h +++ b/src/tbf.h @@ -326,6 +326,12 @@ inline time_t gprs_rlcmac_tbf::created_ts() const return m_created_ts; } +struct gprs_rlcmac_dl_tbf : public gprs_rlcmac_tbf { +}; + +struct gprs_rlcmac_ul_tbf : public gprs_rlcmac_tbf { +}; + #endif #ifdef __cplusplus -- 1.8.4.2
Start returning the special type instead of the base gprs_rlcmac_tbf when retrieving a TBF. Ticket: SYS#389 Sponsored-by: On-Waves ehf --- src/bts.cpp | 36 ++++++++++++++++++------------------ src/bts.h | 16 ++++++++-------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/bts.cpp b/src/bts.cpp index f614dab..68d72f2 100644 --- a/src/bts.cpp +++ b/src/bts.cpp @@ -217,15 +217,15 @@ int BTS::add_paging(uint8_t chan_needed, uint8_t *identity_lv) } /* search for active downlink tbf */ -gprs_rlcmac_tbf *BTS::dl_tbf_by_tlli(uint32_t tlli) +gprs_rlcmac_dl_tbf *BTS::dl_tbf_by_tlli(uint32_t tlli) { - return tbf_by_tlli(tlli, GPRS_RLCMAC_DL_TBF); + return (gprs_rlcmac_dl_tbf *)tbf_by_tlli(tlli, GPRS_RLCMAC_DL_TBF); } /* search for active uplink tbf */ -gprs_rlcmac_tbf *BTS::ul_tbf_by_tlli(uint32_t tlli) +gprs_rlcmac_ul_tbf *BTS::ul_tbf_by_tlli(uint32_t tlli) { - return tbf_by_tlli(tlli, GPRS_RLCMAC_UL_TBF); + return (gprs_rlcmac_ul_tbf *)tbf_by_tlli(tlli, GPRS_RLCMAC_UL_TBF); } /* search for active downlink or uplink tbf */ @@ -250,9 +250,9 @@ gprs_rlcmac_tbf *BTS::tbf_by_tlli(uint32_t tlli, enum gprs_rlcmac_tbf_direction return NULL; } -gprs_rlcmac_tbf *BTS::dl_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts) +gprs_rlcmac_dl_tbf *BTS::dl_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts) { - struct gprs_rlcmac_tbf *tbf; + struct gprs_rlcmac_dl_tbf *tbf; /* only one TBF can poll on specific TS/FN, because scheduler can only * schedule one downlink control block (with polling) at a FN per TS */ @@ -262,14 +262,14 @@ gprs_rlcmac_tbf *BTS::dl_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts) && tbf->poll_fn == fn && tbf->trx->trx_no == trx && tbf->control_ts == ts) { OSMO_ASSERT(tbf->direction == GPRS_RLCMAC_DL_TBF); - return tbf; + return (gprs_rlcmac_dl_tbf *)tbf; } } return NULL; } -gprs_rlcmac_tbf *BTS::ul_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts) +gprs_rlcmac_ul_tbf *BTS::ul_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts) { - struct gprs_rlcmac_tbf *tbf; + struct gprs_rlcmac_ul_tbf *tbf; /* only one TBF can poll on specific TS/FN, because scheduler can only * schedule one downlink control block (with polling) at a FN per TS */ @@ -279,22 +279,22 @@ gprs_rlcmac_tbf *BTS::ul_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts) && tbf->poll_fn == fn && tbf->trx->trx_no == trx && tbf->control_ts == ts) { OSMO_ASSERT(tbf->direction == GPRS_RLCMAC_UL_TBF); - return tbf; + return (gprs_rlcmac_ul_tbf *)tbf; } } return NULL; } /* lookup downlink TBF Entity (by TFI) */ -gprs_rlcmac_tbf *BTS::dl_tbf_by_tfi(uint8_t tfi, uint8_t trx) +gprs_rlcmac_dl_tbf *BTS::dl_tbf_by_tfi(uint8_t tfi, uint8_t trx) { - return tbf_by_tfi(tfi, trx, GPRS_RLCMAC_DL_TBF); + return (gprs_rlcmac_dl_tbf *)tbf_by_tfi(tfi, trx, GPRS_RLCMAC_DL_TBF); } /* lookup uplink TBF Entity (by TFI) */ -gprs_rlcmac_tbf *BTS::ul_tbf_by_tfi(uint8_t tfi, uint8_t trx) +gprs_rlcmac_ul_tbf *BTS::ul_tbf_by_tfi(uint8_t tfi, uint8_t trx) { - return tbf_by_tfi(tfi, trx, GPRS_RLCMAC_UL_TBF); + return (gprs_rlcmac_ul_tbf *)tbf_by_tfi(tfi, trx, GPRS_RLCMAC_UL_TBF); } /* lookup TBF Entity (by TFI) */ @@ -1047,12 +1047,12 @@ gprs_rlcmac_tbf *gprs_rlcmac_pdch::tbf_from_list_by_tfi(struct llist_head *tbf_l return NULL; } -gprs_rlcmac_tbf *gprs_rlcmac_pdch::ul_tbf_by_tfi(uint8_t tfi) +gprs_rlcmac_ul_tbf *gprs_rlcmac_pdch::ul_tbf_by_tfi(uint8_t tfi) { - return tbf_from_list_by_tfi(&bts_data()->ul_tbfs, tfi, GPRS_RLCMAC_UL_TBF); + return (gprs_rlcmac_ul_tbf *)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) +gprs_rlcmac_dl_tbf *gprs_rlcmac_pdch::dl_tbf_by_tfi(uint8_t tfi) { - return tbf_from_list_by_tfi(&bts_data()->dl_tbfs, tfi, GPRS_RLCMAC_DL_TBF); + return (gprs_rlcmac_dl_tbf *)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 a0b808e..8f99942 100644 --- a/src/bts.h +++ b/src/bts.h @@ -62,8 +62,8 @@ struct gprs_rlcmac_pdch { BTS *bts() const; uint8_t trx_no() const; - struct gprs_rlcmac_tbf *ul_tbf_by_tfi(uint8_t tfi); - struct gprs_rlcmac_tbf *dl_tbf_by_tfi(uint8_t tfi); + struct gprs_rlcmac_ul_tbf *ul_tbf_by_tfi(uint8_t tfi); + struct gprs_rlcmac_dl_tbf *dl_tbf_by_tfi(uint8_t tfi); #endif uint8_t m_is_enabled; /* TS is enabled */ @@ -193,12 +193,12 @@ public: /** add paging to paging queue(s) */ int add_paging(uint8_t chan_needed, uint8_t *identity_lv); - gprs_rlcmac_tbf *dl_tbf_by_tlli(uint32_t tlli); - gprs_rlcmac_tbf *ul_tbf_by_tlli(uint32_t tlli); - gprs_rlcmac_tbf *dl_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts); - gprs_rlcmac_tbf *ul_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts); - gprs_rlcmac_tbf *dl_tbf_by_tfi(uint8_t tfi, uint8_t trx); - gprs_rlcmac_tbf *ul_tbf_by_tfi(uint8_t tfi, uint8_t trx); + gprs_rlcmac_dl_tbf *dl_tbf_by_tlli(uint32_t tlli); + gprs_rlcmac_ul_tbf *ul_tbf_by_tlli(uint32_t tlli); + gprs_rlcmac_dl_tbf *dl_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts); + gprs_rlcmac_ul_tbf *ul_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts); + gprs_rlcmac_dl_tbf *dl_tbf_by_tfi(uint8_t tfi, uint8_t trx); + gprs_rlcmac_ul_tbf *ul_tbf_by_tfi(uint8_t tfi, uint8_t trx); int tfi_find_free(enum gprs_rlcmac_tbf_direction dir, uint8_t *_trx, int8_t use_trx); -- 1.8.4.2
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(-) 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); -- 1.8.4.2
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
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.
UL and DL tbfs are used in very separate parts and are not the same thing so split the alloc function and use the UL/DL version throughout the code. Ticket: SYS#389 Sponsored-by: On-Waves ehf --- src/bts.cpp | 2 +- src/tbf.cpp | 102 +++++++++++++++++++++++++++++++++------------- src/tbf.h | 9 +++- tests/alloc/AllocTest.cpp | 31 +++++++++----- tests/tbf/TbfTest.cpp | 8 ++-- 5 files changed, 106 insertions(+), 46 deletions(-) diff --git a/src/bts.cpp b/src/bts.cpp index 8489431..5bf139d 100644 --- a/src/bts.cpp +++ b/src/bts.cpp @@ -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 = (gprs_rlcmac_ul_tbf *)tbf_alloc(&m_bts, NULL, GPRS_RLCMAC_UL_TBF, tfi, trx_no, 0, 1); + tbf = tbf_alloc_ul_tbf(&m_bts, NULL, tfi, trx_no, 0, 1); if (!tbf) { LOGP(DRLCMAC, LOGL_NOTICE, "No PDCH resource\n"); /* FIXME: send reject */ diff --git a/src/tbf.cpp b/src/tbf.cpp index 15301a1..1201642 100644 --- a/src/tbf.cpp +++ b/src/tbf.cpp @@ -165,7 +165,7 @@ static int tbf_new_dl_assignment(struct gprs_rlcmac_bts *bts, return -EBUSY; } /* set number of downlink slots according to multislot class */ - tbf = tbf_alloc(bts, tbf, GPRS_RLCMAC_DL_TBF, tfi, trx, ms_class, ss); + tbf = tbf_alloc_dl_tbf(bts, tbf, tfi, trx, ms_class, ss); if (!tbf) { LOGP(DRLCMAC, LOGL_NOTICE, "No PDCH resource\n"); /* FIXME: send reject */ @@ -231,7 +231,7 @@ gprs_rlcmac_ul_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts, return NULL; } /* use multislot class of downlink TBF */ - tbf = (gprs_rlcmac_ul_tbf *)tbf_alloc(bts, dl_tbf, GPRS_RLCMAC_UL_TBF, tfi, trx, ms_class, 0); + tbf = tbf_alloc_ul_tbf(bts, dl_tbf, tfi, trx, ms_class, 0); if (!tbf) { LOGP(DRLCMAC, LOGL_NOTICE, "No PDCH resource\n"); /* FIXME: send reject */ @@ -479,33 +479,20 @@ void gprs_rlcmac_tbf::poll_timeout() LOGP(DRLCMAC, LOGL_ERROR, "- Poll Timeout, but no event!\n"); } -struct gprs_rlcmac_tbf *tbf_alloc(struct gprs_rlcmac_bts *bts, - struct gprs_rlcmac_tbf *old_tbf, enum gprs_rlcmac_tbf_direction dir, - uint8_t tfi, uint8_t trx, +static int setup_tbf(struct gprs_rlcmac_tbf *tbf, struct gprs_rlcmac_bts *bts, + struct gprs_rlcmac_tbf *old_tbf, uint8_t tfi, uint8_t trx, uint8_t ms_class, uint8_t single_slot) { - struct gprs_rlcmac_tbf *tbf; int rc; - LOGP(DRLCMAC, LOGL_DEBUG, "********** TBF starts here **********\n"); - LOGP(DRLCMAC, LOGL_INFO, "Allocating %s TBF: TFI=%d TRX=%d " - "MS_CLASS=%d\n", (dir == GPRS_RLCMAC_UL_TBF) ? "UL" : "DL", - tfi, trx, ms_class); - if (trx >= 8 || tfi >= 32) - return NULL; - - if (dir == GPRS_RLCMAC_UL_TBF) - tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_ul_tbf); - else - tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_dl_tbf); + return -1; if (!tbf) - return NULL; + return -1; tbf->m_created_ts = time(NULL); tbf->bts = bts->bts; - tbf->direction = dir; tbf->m_tfi = tfi; tbf->trx = &bts->trx[trx]; tbf->ms_class = ms_class; @@ -514,16 +501,14 @@ struct gprs_rlcmac_tbf *tbf_alloc(struct gprs_rlcmac_bts *bts, single_slot); /* if no resource */ if (rc < 0) { - talloc_free(tbf); - return NULL; + return -1; } /* assign control ts */ tbf->control_ts = 0xff; rc = tbf_assign_control_ts(tbf); /* if no resource */ if (rc < 0) { - talloc_free(tbf); - return NULL; + return -1; } /* set timestamp */ @@ -532,14 +517,73 @@ struct gprs_rlcmac_tbf *tbf_alloc(struct gprs_rlcmac_bts *bts, gettimeofday(&tbf->meas.dl_loss_tv, NULL); tbf->m_llc.init(); - if (dir == GPRS_RLCMAC_UL_TBF) { - llist_add(&tbf->list, &bts->ul_tbfs); - tbf->bts->tbf_ul_created(); - } else { - llist_add(&tbf->list, &bts->dl_tbfs); - tbf->bts->tbf_dl_created(); + return 0; +} + + +struct gprs_rlcmac_ul_tbf *tbf_alloc_ul_tbf(struct gprs_rlcmac_bts *bts, + struct gprs_rlcmac_tbf *old_tbf, uint8_t tfi, uint8_t trx, + uint8_t ms_class, uint8_t single_slot) +{ + struct gprs_rlcmac_ul_tbf *tbf; + int rc; + + LOGP(DRLCMAC, LOGL_DEBUG, "********** TBF starts here **********\n"); + LOGP(DRLCMAC, LOGL_INFO, "Allocating %s TBF: TFI=%d TRX=%d " + "MS_CLASS=%d\n", "UL", tfi, trx, ms_class); + + if (trx >= 8 || tfi >= 32) + return NULL; + + tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_ul_tbf); + + if (!tbf) + return NULL; + + tbf->direction = GPRS_RLCMAC_UL_TBF; + rc = setup_tbf(tbf, bts, old_tbf, tfi, trx, ms_class, single_slot); + /* if no resource */ + if (rc < 0) { + talloc_free(tbf); + return NULL; + } + + llist_add(&tbf->list, &bts->ul_tbfs); + tbf->bts->tbf_ul_created(); + + return tbf; +} + +struct gprs_rlcmac_dl_tbf *tbf_alloc_dl_tbf(struct gprs_rlcmac_bts *bts, + struct gprs_rlcmac_tbf *old_tbf, uint8_t tfi, uint8_t trx, + uint8_t ms_class, uint8_t single_slot) +{ + struct gprs_rlcmac_dl_tbf *tbf; + int rc; + + LOGP(DRLCMAC, LOGL_DEBUG, "********** TBF starts here **********\n"); + LOGP(DRLCMAC, LOGL_INFO, "Allocating %s TBF: TFI=%d TRX=%d " + "MS_CLASS=%d\n", "DL", tfi, trx, ms_class); + + if (trx >= 8 || tfi >= 32) + return NULL; + + tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_dl_tbf); + + if (!tbf) + return NULL; + + tbf->direction = GPRS_RLCMAC_DL_TBF; + rc = setup_tbf(tbf, bts, old_tbf, tfi, trx, ms_class, single_slot); + /* if no resource */ + if (rc < 0) { + talloc_free(tbf); + return NULL; } + llist_add(&tbf->list, &bts->dl_tbfs); + tbf->bts->tbf_dl_created(); + return tbf; } diff --git a/src/tbf.h b/src/tbf.h index d650021..4d20987 100644 --- a/src/tbf.h +++ b/src/tbf.h @@ -263,9 +263,14 @@ 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); -struct gprs_rlcmac_tbf *tbf_alloc(struct gprs_rlcmac_bts *bts, +struct gprs_rlcmac_ul_tbf *tbf_alloc_ul_tbf(struct gprs_rlcmac_bts *bts, struct gprs_rlcmac_tbf *old_tbf, - enum gprs_rlcmac_tbf_direction dir, uint8_t tfi, uint8_t trx, + uint8_t tfi, uint8_t trx, + uint8_t ms_class, uint8_t single_slot); + +struct gprs_rlcmac_dl_tbf *tbf_alloc_dl_tbf(struct gprs_rlcmac_bts *bts, + struct gprs_rlcmac_tbf *old_tbf, + uint8_t tfi, uint8_t trx, uint8_t ms_class, uint8_t single_slot); void tbf_free(struct gprs_rlcmac_tbf *tbf); diff --git a/tests/alloc/AllocTest.cpp b/tests/alloc/AllocTest.cpp index 80e8c34..830ca90 100644 --- a/tests/alloc/AllocTest.cpp +++ b/tests/alloc/AllocTest.cpp @@ -36,6 +36,17 @@ extern "C" { void *tall_pcu_ctx; int16_t spoof_mnc = 0, spoof_mcc = 0; +static gprs_rlcmac_tbf *tbf_alloc(struct gprs_rlcmac_bts *bts, + struct gprs_rlcmac_tbf *old_tbf, gprs_rlcmac_tbf_direction dir, + uint8_t tfi, uint8_t trx, + uint8_t ms_class, uint8_t single_slot) +{ + if (dir == GPRS_RLCMAC_UL_TBF) + return tbf_alloc_ul_tbf(bts, old_tbf, tfi, trx, ms_class, single_slot); + else + return tbf_alloc_dl_tbf(bts, old_tbf, tfi, trx, ms_class, single_slot); +} + static void test_alloc_a(gprs_rlcmac_tbf_direction dir, const int count) { int tfi; @@ -136,14 +147,14 @@ static void test_alloc_b(int ms_class) tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1); OSMO_ASSERT(tfi >= 0); - ul_tbf = tbf_alloc(bts, NULL, GPRS_RLCMAC_UL_TBF, tfi, trx_no, ms_class, 1); + ul_tbf = tbf_alloc_ul_tbf(bts, NULL, tfi, trx_no, ms_class, 1); OSMO_ASSERT(ul_tbf); dump_assignment(ul_tbf, "UL"); /* assume final ack has not been sent */ tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1); OSMO_ASSERT(tfi >= 0); - dl_tbf = tbf_alloc(bts, ul_tbf, GPRS_RLCMAC_DL_TBF, tfi, trx_no, ms_class, 0); + dl_tbf = tbf_alloc_dl_tbf(bts, ul_tbf, tfi, trx_no, ms_class, 0); OSMO_ASSERT(dl_tbf); dump_assignment(dl_tbf, "DL"); @@ -177,7 +188,7 @@ static void test_alloc_b(int ms_class) tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1); OSMO_ASSERT(tfi >= 0); - dl_tbf = tbf_alloc(bts, NULL, GPRS_RLCMAC_DL_TBF, tfi, trx_no, ms_class, 1); + dl_tbf = tbf_alloc_dl_tbf(bts, NULL, tfi, trx_no, ms_class, 1); dl_tbf->m_tlli = 0x23; dl_tbf->m_tlli_valid = true; OSMO_ASSERT(dl_tbf); @@ -185,7 +196,7 @@ static void test_alloc_b(int ms_class) tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1); OSMO_ASSERT(tfi >= 0); - ul_tbf = tbf_alloc(bts, dl_tbf, GPRS_RLCMAC_UL_TBF, tfi, trx_no, ms_class, 0); + ul_tbf = tbf_alloc_ul_tbf(bts, dl_tbf, tfi, trx_no, ms_class, 0); ul_tbf->m_tlli = 0x23; ul_tbf->m_tlli_valid = true; ul_tbf->dir.ul.contention_resolution_done = 1; @@ -226,14 +237,14 @@ static void test_alloc_b(int ms_class) tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1); OSMO_ASSERT(tfi >= 0); - ul_tbf = tbf_alloc(bts, NULL, GPRS_RLCMAC_UL_TBF, tfi, trx_no, ms_class, 0); + ul_tbf = tbf_alloc_ul_tbf(bts, NULL, tfi, trx_no, ms_class, 0); OSMO_ASSERT(ul_tbf); dump_assignment(ul_tbf, "UL"); /* assume final ack has not been sent */ tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1); OSMO_ASSERT(tfi >= 0); - dl_tbf = tbf_alloc(bts, ul_tbf, GPRS_RLCMAC_DL_TBF, tfi, trx_no, ms_class, 0); + dl_tbf = tbf_alloc_dl_tbf(bts, ul_tbf, tfi, trx_no, ms_class, 0); OSMO_ASSERT(dl_tbf); dump_assignment(dl_tbf, "DL"); @@ -290,13 +301,13 @@ static void test_alloc_b(bool ts0, bool ts1, bool ts2, bool ts3, bool ts4, bool tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1); OSMO_ASSERT(tfi >= 0); - ul_tbf = tbf_alloc(bts, NULL, GPRS_RLCMAC_UL_TBF, tfi, trx_no, ms_class, 1); + ul_tbf = tbf_alloc_ul_tbf(bts, NULL, tfi, trx_no, ms_class, 1); OSMO_ASSERT(ul_tbf); /* assume final ack has not been sent */ tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1); OSMO_ASSERT(tfi >= 0); - dl_tbf = tbf_alloc(bts, ul_tbf, GPRS_RLCMAC_DL_TBF, tfi, trx_no, ms_class, 0); + dl_tbf = tbf_alloc_dl_tbf(bts, ul_tbf, tfi, trx_no, ms_class, 0); OSMO_ASSERT(dl_tbf); /* verify that both are on the same ts */ @@ -333,14 +344,14 @@ static void test_alloc_b(bool ts0, bool ts1, bool ts2, bool ts3, bool ts4, bool tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1); OSMO_ASSERT(tfi >= 0); - dl_tbf = tbf_alloc(bts, NULL, GPRS_RLCMAC_DL_TBF, tfi, trx_no, ms_class, 1); + dl_tbf = tbf_alloc_dl_tbf(bts, NULL, tfi, trx_no, ms_class, 1); OSMO_ASSERT(dl_tbf); dl_tbf->m_tlli = 0x23; dl_tbf->m_tlli_valid = true; tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1); OSMO_ASSERT(tfi >= 0); - ul_tbf = tbf_alloc(bts, dl_tbf, GPRS_RLCMAC_UL_TBF, tfi, trx_no, ms_class, 0); + ul_tbf = tbf_alloc_ul_tbf(bts, dl_tbf, tfi, trx_no, ms_class, 0); OSMO_ASSERT(ul_tbf); ul_tbf->m_tlli = 0x23; ul_tbf->m_tlli_valid = true; diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp index 38975f9..decf4d9 100644 --- a/tests/tbf/TbfTest.cpp +++ b/tests/tbf/TbfTest.cpp @@ -44,16 +44,16 @@ static void test_tbf_tlli_update() /* * Make a uplink and downlink allocation */ - gprs_rlcmac_tbf *dl_tbf = tbf_alloc(the_bts.bts_data(), - NULL, GPRS_RLCMAC_DL_TBF, 0, + gprs_rlcmac_tbf *dl_tbf = tbf_alloc_dl_tbf(the_bts.bts_data(), + NULL, 0, 0, 0, 0); dl_tbf->update_tlli(0x2342); dl_tbf->tlli_mark_valid(); dl_tbf->ta = 4; the_bts.timing_advance()->remember(0x2342, dl_tbf->ta); - gprs_rlcmac_tbf *ul_tbf = tbf_alloc(the_bts.bts_data(), - ul_tbf, GPRS_RLCMAC_UL_TBF, 0, + gprs_rlcmac_tbf *ul_tbf = tbf_alloc_ul_tbf(the_bts.bts_data(), + ul_tbf, 0, 0, 0, 0); ul_tbf->update_tlli(0x2342); ul_tbf->tlli_mark_valid(); -- 1.8.4.2
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.
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 -- - Daniel Willmann <dwillmann@sysmocom.de> http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Schivelbeiner Str. 5 * 10439 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
participants (2)
-
Daniel Willmann -
Holger Hans Peter Freyther