From: Holger Hans Peter Freyther holger@moiji-mobile.com
Call things by what they do and this function doesn't allocate anything but it is searching for the first unallocated tbf index. --- src/gprs_bssgp_pcu.cpp | 2 +- src/gprs_rlcmac.cpp | 3 ++- src/gprs_rlcmac.h | 3 ++- src/gprs_rlcmac_data.cpp | 4 ++-- 4 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp index 6f67f52..4833af8 100644 --- a/src/gprs_bssgp_pcu.cpp +++ b/src/gprs_bssgp_pcu.cpp @@ -233,7 +233,7 @@ static int gprs_bssgp_pcu_rx_dl_ud(struct msgb *msg, struct tlv_parsed *tp) }
// Create new TBF (any TRX) - tfi = tfi_alloc(the_pcu.bts, GPRS_RLCMAC_DL_TBF, &trx, use_trx); + tfi = tfi_find_free(the_pcu.bts, GPRS_RLCMAC_DL_TBF, &trx, use_trx); if (tfi < 0) { LOGP(DRLCMAC, LOGL_NOTICE, "No PDCH resource\n"); /* FIXME: send reject */ diff --git a/src/gprs_rlcmac.cpp b/src/gprs_rlcmac.cpp index 8874983..913ae48 100644 --- a/src/gprs_rlcmac.cpp +++ b/src/gprs_rlcmac.cpp @@ -2,6 +2,7 @@ * * Copyright (C) 2012 Ivan Klyuchnikov * Copyright (C) 2012 Andreas Eversberg jolly@eversberg.eu + * Copyright (C) 2013 by Holger Hans Peter Freyther * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -176,7 +177,7 @@ void debug_diagram(int diag, const char *format, ...) /* FIXME: spread ressources over multiple TRX. Also add option to use same * TRX in case of existing TBF for TLLI in the other direction. */ /* search for free TFI and return TFI, TRX */ -int tfi_alloc(struct gprs_rlcmac_bts *bts, enum gprs_rlcmac_tbf_direction dir, +int tfi_find_free(struct gprs_rlcmac_bts *bts, enum gprs_rlcmac_tbf_direction dir, uint8_t *_trx, int8_t use_trx) { struct gprs_rlcmac_pdch *pdch; diff --git a/src/gprs_rlcmac.h b/src/gprs_rlcmac.h index ac437f5..4329cb8 100644 --- a/src/gprs_rlcmac.h +++ b/src/gprs_rlcmac.h @@ -1,6 +1,7 @@ /* gprs_rlcmac.h * * Copyright (C) 2012 Ivan Klyuchnikov + * Copyright (C) 2013 by Holger Hans Peter Freyther * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -309,7 +310,7 @@ int sba_alloc(uint8_t *_trx, uint8_t *_ts, uint32_t *_fn, uint8_t ta);
struct gprs_rlcmac_sba *sba_find(uint8_t trx, uint8_t ts, uint32_t fn);
-int tfi_alloc(struct gprs_rlcmac_bts *bts, enum gprs_rlcmac_tbf_direction dir, +int tfi_find_free(struct gprs_rlcmac_bts *bts, enum gprs_rlcmac_tbf_direction dir, uint8_t *_trx, int8_t use_trx);
struct gprs_rlcmac_tbf *tbf_alloc(struct gprs_rlcmac_bts *bts, diff --git a/src/gprs_rlcmac_data.cpp b/src/gprs_rlcmac_data.cpp index 4d5acb9..ff5274f 100644 --- a/src/gprs_rlcmac_data.cpp +++ b/src/gprs_rlcmac_data.cpp @@ -239,7 +239,7 @@ static struct gprs_rlcmac_tbf *alloc_ul_tbf(int8_t use_trx, uint8_t ms_class, uint8_t tfi;
/* create new TBF, use sme TRX as DL TBF */ - tfi = tfi_alloc(bts, GPRS_RLCMAC_UL_TBF, &trx, use_trx); + tfi = tfi_find_free(bts, GPRS_RLCMAC_UL_TBF, &trx, use_trx); if (tfi < 0) { LOGP(DRLCMAC, LOGL_NOTICE, "No PDCH ressource\n"); /* FIXME: send reject */ @@ -1174,7 +1174,7 @@ int gprs_rlcmac_rcv_rach(uint8_t ra, uint32_t Fn, int16_t qta) "(AGCH)\n"); } else { // Create new TBF - tfi = tfi_alloc(bts, GPRS_RLCMAC_UL_TBF, &trx, -1); + tfi = tfi_find_free(bts, GPRS_RLCMAC_UL_TBF, &trx, -1); if (tfi < 0) { LOGP(DRLCMAC, LOGL_NOTICE, "No PDCH ressource\n"); /* FIXME: send reject */
From: Holger Hans Peter Freyther holger@moiji-mobile.com
--- src/gprs_rlcmac.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/gprs_rlcmac.cpp b/src/gprs_rlcmac.cpp index 913ae48..1d1a8c6 100644 --- a/src/gprs_rlcmac.cpp +++ b/src/gprs_rlcmac.cpp @@ -36,7 +36,7 @@ struct gprs_ms_multislot_class { uint8_t type; /* Type of Mobile */ };
-struct gprs_ms_multislot_class gprs_ms_multislot_class[32] = { +static const struct gprs_ms_multislot_class gprs_ms_multislot_class[32] = { /* M-S Class Rx Tx Sum Tta Ttb Tra Trb Type */ /* N/A */ { MS_NA,MS_NA, MS_NA, MS_NA, MS_NA, MS_NA, MS_NA, MS_NA }, /* 1 */ { 1, 1, 2, 3, 2, 4, 2, 1 }, @@ -467,14 +467,14 @@ int alloc_algorithm_b(struct gprs_rlcmac_bts *bts, struct gprs_rlcmac_tbf *tbf, uint32_t cust, uint8_t single) { struct gprs_rlcmac_pdch *pdch; - struct gprs_ms_multislot_class *ms_class; + const struct gprs_ms_multislot_class *ms_class; uint8_t Rx, Tx, Sum; /* Maximum Number of Slots: RX, Tx, Sum Rx+Tx */ uint8_t Tta, Ttb, Tra, Trb, Tt, Tr; /* Minimum Number of Slots */ uint8_t Type; /* Type of Mobile */ uint8_t rx_win_min = 0, rx_win_max = 7; uint8_t tx_win_min, tx_win_max, tx_range; uint8_t rx_window = 0, tx_window = 0; - const char *digit[10] = { "0","1","2","3","4","5","6","7","8","9" }; + static const char *digit[10] = { "0","1","2","3","4","5","6","7","8","9" }; int8_t usf[8] = { -1, -1, -1, -1, -1, -1, -1, -1 }; /* must be signed */ int8_t tsc = -1; /* must be signed */ int8_t first_common_ts = -1;
From: Holger Hans Peter Freyther holger@moiji-mobile.com
--- src/gprs_rlcmac.h | 2 +- src/gprs_rlcmac_data.cpp | 6 +++--- src/pcu_l1_if.cpp | 2 +- src/pcu_l1_if.h | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/gprs_rlcmac.h b/src/gprs_rlcmac.h index 4329cb8..8de7417 100644 --- a/src/gprs_rlcmac.h +++ b/src/gprs_rlcmac.h @@ -392,7 +392,7 @@ struct msgb *gprs_rlcmac_send_packet_downlink_assignment( struct gprs_rlcmac_tbf *tbf, uint32_t fn);
void gprs_rlcmac_trigger_downlink_assignment(struct gprs_rlcmac_tbf *tbf, - struct gprs_rlcmac_tbf *old_tbf, char *imsi); + struct gprs_rlcmac_tbf *old_tbf, const char *imsi);
int gprs_rlcmac_downlink_ack(struct gprs_rlcmac_tbf *tbf, uint8_t final, uint8_t ssn, uint8_t *rbb); diff --git a/src/gprs_rlcmac_data.cpp b/src/gprs_rlcmac_data.cpp index ff5274f..294753d 100644 --- a/src/gprs_rlcmac_data.cpp +++ b/src/gprs_rlcmac_data.cpp @@ -74,7 +74,7 @@ struct rlc_li_field { }
static void gprs_rlcmac_downlink_assignment(gprs_rlcmac_tbf *tbf, uint8_t poll, - char *imsi); + const char *imsi);
static int gprs_rlcmac_diag(struct gprs_rlcmac_tbf *tbf) { @@ -1819,7 +1819,7 @@ struct msgb *gprs_rlcmac_send_packet_downlink_assignment( }
static void gprs_rlcmac_downlink_assignment(gprs_rlcmac_tbf *tbf, uint8_t poll, - char *imsi) + const char *imsi) { struct gprs_rlcmac_bts *bts = gprs_rlcmac_bts; int plen; @@ -1840,7 +1840,7 @@ static void gprs_rlcmac_downlink_assignment(gprs_rlcmac_tbf *tbf, uint8_t poll,
/* depending on the current TBF, we assign on PACCH or AGCH */ void gprs_rlcmac_trigger_downlink_assignment(struct gprs_rlcmac_tbf *tbf, - struct gprs_rlcmac_tbf *old_tbf, char *imsi) + struct gprs_rlcmac_tbf *old_tbf, const char *imsi) { #ifdef DEBUG_DL_ASS_IDLE strncpy(debug_imsi, imsi); diff --git a/src/pcu_l1_if.cpp b/src/pcu_l1_if.cpp index 43bd36e..218dc23 100644 --- a/src/pcu_l1_if.cpp +++ b/src/pcu_l1_if.cpp @@ -174,7 +174,7 @@ void pcu_l1if_tx_agch(bitvec * block, int plen) pcu_tx_data_req(0, 0, PCU_IF_SAPI_AGCH, 0, 0, 0, data, 23); }
-void pcu_l1if_tx_pch(bitvec * block, int plen, char *imsi) +void pcu_l1if_tx_pch(bitvec * block, int plen, const char *imsi) { uint8_t data[23+3]; /* prefix PLEN */
diff --git a/src/pcu_l1_if.h b/src/pcu_l1_if.h index 715f89f..f01b95a 100644 --- a/src/pcu_l1_if.h +++ b/src/pcu_l1_if.h @@ -40,7 +40,7 @@ void pcu_l1if_tx_ptcch(msgb *msg, uint8_t trx, uint8_t ts, uint16_t arfcn, uint32_t fn, uint8_t block_nr); void pcu_l1if_tx_agch(bitvec * block, int len);
-void pcu_l1if_tx_pch(bitvec * block, int plen, char *imsi); +void pcu_l1if_tx_pch(bitvec * block, int plen, const char *imsi);
int pcu_l1if_open(void); void pcu_l1if_close(void);
On Sun, Aug 25, 2013 at 01:01:36PM +0200, Holger Hans Peter Freyther wrote:
From: Holger Hans Peter Freyther holger@moiji-mobile.com
Ivan, ping?
On Sun, Aug 25, 2013 at 01:01:34PM +0200, Holger Hans Peter Freyther wrote:
Hi,
Call things by what they do and this function doesn't allocate anything but it is searching for the first unallocated tbf index.
as part of cleaning up and adding structure (and on the way to eliminate the various hidden errors) I came across tfi_alloc. There are three places that mostly do (the topic of copy and paste in the pcu code is another topic):
tfi = tfi_alloc() if (tfi < 0) return; tbf = tbf_alloc(tfi,...); if (!tbf) return;
Now just by the name... we would leak the result of tfi_alloc in the above code. But then again tfi_alloc doesn't allocate anything it is just trying to find an unallocated index.
Which brings me to: Name things by what they do (and update the name when the implementation does change). In the above case if the method is called tfi_find_free I will know that there is no side-effect and that I will not need to call a tfi_free.
cheers holger
On Sun, Aug 25, 2013 at 01:01:34PM +0200, Holger Hans Peter Freyther wrote:
From: Holger Hans Peter Freyther holger@moiji-mobile.com
Hi,
Call things by what they do and this function doesn't allocate anything but it is searching for the first unallocated tbf index.
it appears that this patchset has not ben reviewed/merged. Do you have time to review/test/merge these cleanups?
holger
Hi Holger,
I looked through these patches. Do you want to merge only these three patches or may be it makes sense to merge all patches from zecke/features/tbf-cleanup branch?
2013/9/29 Holger Hans Peter Freyther hfreyther@sysmocom.de:
On Sun, Aug 25, 2013 at 01:01:34PM +0200, Holger Hans Peter Freyther wrote:
From: Holger Hans Peter Freyther holger@moiji-mobile.com
Hi,
Call things by what they do and this function doesn't allocate anything but it is searching for the first unallocated tbf index.
it appears that this patchset has not ben reviewed/merged. Do you have time to review/test/merge these cleanups?
holger
--
- Holger Freyther hfreyther@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
On Tue, Oct 01, 2013 at 10:03:39AM +0400, Ivan Kluchnikov wrote:
Hi Holger,
I looked through these patches. Do you want to merge only these three patches or may be it makes sense to merge all patches from zecke/features/tbf-cleanup branch?
I prefer to establish a culture of review and feedback. I hope that by cleaning up this code in public the original author and others reading the mailinglist will reflect on the code and maybe improve.
holger
Ok, patches were merged to master and now I am ready for new patchset.
2013/10/1 Holger Hans Peter Freyther hfreyther@sysmocom.de:
On Tue, Oct 01, 2013 at 10:03:39AM +0400, Ivan Kluchnikov wrote:
Hi Holger,
I looked through these patches. Do you want to merge only these three patches or may be it makes sense to merge all patches from zecke/features/tbf-cleanup branch?
I prefer to establish a culture of review and feedback. I hope that by cleaning up this code in public the original author and others reading the mailinglist will reflect on the code and maybe improve.
holger
--
- Holger Freyther hfreyther@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
osmocom-net-gprs@lists.osmocom.org
-
Holger Hans Peter Freyther -
Ivan Kluchnikov