From: Holger Hans Peter Freyther holger@moiji-mobile.com
One of the issues with not properly re-setting everything is that due the global state it is not clear which variables belong together and how long it exists. Begin with creating a osmo_pcu and moving things into this class.
Think of an organic cell, this commit is introducing the cell wall around it... and defines what is inside and what is outside of it. --- src/gprs_bssgp_pcu.cpp | 56 ++++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 24 deletions(-)
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp index 59185e3..e94b5eb 100644 --- a/src/gprs_bssgp_pcu.cpp +++ b/src/gprs_bssgp_pcu.cpp @@ -21,13 +21,21 @@ #include <gprs_bssgp_pcu.h> #include <pcu_l1_if.h>
+struct osmo_pcu { + struct gprs_nsvc *nsvc; + + int bvc_sig_reset; + int bvc_reset; + int bvc_unblocked; + int exit_on_destroy; +}; + +static struct osmo_pcu the_pcu = { 0, }; + struct sgsn_instance *sgsn; extern void *tall_pcu_ctx; struct bssgp_bvc_ctx *bctx = NULL; -struct gprs_nsvc *nsvc = NULL; -static int bvc_sig_reset = 0, bvc_reset = 0, bvc_unblocked = 0; extern uint16_t spoof_mcc, spoof_mnc; -static int exit_on_destroy = 0;
struct osmo_timer_list bvc_timer;
@@ -353,10 +361,10 @@ int gprs_bssgp_pcu_rx_sign(struct msgb *msg, struct tlv_parsed *tp, struct bssgp break; case BSSGP_PDUT_BVC_RESET_ACK: LOGP(DBSSGP, LOGL_DEBUG, "rx BSSGP_PDUT_BVC_RESET_ACK\n"); - if (!bvc_sig_reset) - bvc_sig_reset = 1; + if (!the_pcu.bvc_sig_reset) + the_pcu.bvc_sig_reset = 1; else - bvc_reset = 1; + the_pcu.bvc_reset = 1; bvc_timeout(NULL); break; case BSSGP_PDUT_PAGING_PS: @@ -380,7 +388,7 @@ int gprs_bssgp_pcu_rx_sign(struct msgb *msg, struct tlv_parsed *tp, struct bssgp break; case BSSGP_PDUT_BVC_UNBLOCK_ACK: LOGP(DBSSGP, LOGL_DEBUG, "rx BSSGP_PDUT_BVC_UNBLOCK_ACK\n"); - bvc_unblocked = 1; + the_pcu.bvc_unblocked = 1; bvc_timeout(NULL); break; case BSSGP_PDUT_SGSN_INVOKE_TRACE: @@ -496,7 +504,7 @@ static int nsvc_signal_cb(unsigned int subsys, unsigned int signal, return -EINVAL;
nssd = (struct ns_signal_data *)signal_data; - if (nssd->nsvc != nsvc) { + if (nssd->nsvc != the_pcu.nsvc) { LOGP(DPCU, LOGL_ERROR, "Signal received of unknown NSVC\n"); return -EINVAL; } @@ -506,10 +514,10 @@ static int nsvc_signal_cb(unsigned int subsys, unsigned int signal, if (!nsvc_unblocked) { nsvc_unblocked = 1; LOGP(DPCU, LOGL_NOTICE, "NS-VC %d is unblocked.\n", - nsvc->nsvci); - bvc_sig_reset = 0; - bvc_reset = 0; - bvc_unblocked = 0; + the_pcu.nsvc->nsvci); + the_pcu.bvc_sig_reset = 0; + the_pcu.bvc_reset = 0; + the_pcu.bvc_unblocked = 0; bvc_timeout(NULL); } break; @@ -517,9 +525,9 @@ static int nsvc_signal_cb(unsigned int subsys, unsigned int signal, if (nsvc_unblocked) { nsvc_unblocked = 0; osmo_timer_del(&bvc_timer); - bvc_sig_reset = 0; - bvc_reset = 0; - bvc_unblocked = 0; + the_pcu.bvc_sig_reset = 0; + the_pcu.bvc_reset = 0; + the_pcu.bvc_unblocked = 0; LOGP(DPCU, LOGL_NOTICE, "NS-VC is blocked.\n"); } break; @@ -545,14 +553,14 @@ static void bvc_timeout(void *_priv) { struct gprs_rlcmac_bts *bts = gprs_rlcmac_bts;
- if (!bvc_sig_reset) { + if (!the_pcu.bvc_sig_reset) { LOGP(DBSSGP, LOGL_INFO, "Sending reset on BVCI 0\n"); bssgp_tx_bvc_reset(bctx, 0, BSSGP_CAUSE_OML_INTERV); osmo_timer_schedule(&bvc_timer, 1, 0); return; }
- if (!bvc_reset) { + if (!the_pcu.bvc_reset) { LOGP(DBSSGP, LOGL_INFO, "Sending reset on BVCI %d\n", bctx->bvci); bssgp_tx_bvc_reset(bctx, bctx->bvci, BSSGP_CAUSE_OML_INTERV); @@ -560,7 +568,7 @@ static void bvc_timeout(void *_priv) return; }
- if (!bvc_unblocked) { + if (!the_pcu.bvc_unblocked) { LOGP(DBSSGP, LOGL_INFO, "Sending unblock on BVCI %d\n", bctx->bvci); bssgp_tx_bvc_unblock(bctx); @@ -609,8 +617,8 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip, dest.sin_port = htons(sgsn_port); dest.sin_addr.s_addr = htonl(sgsn_ip);
- nsvc = gprs_ns_nsip_connect(bssgp_nsi, &dest, nsei, nsvci); - if (!nsvc) { + the_pcu.nsvc = gprs_ns_nsip_connect(bssgp_nsi, &dest, nsei, nsvci); + if (!the_pcu.nsvc) { LOGP(DBSSGP, LOGL_ERROR, "Failed to create NSVCt\n"); gprs_ns_destroy(bssgp_nsi); bssgp_nsi = NULL; @@ -620,7 +628,7 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip, bctx = btsctx_alloc(bvci, nsei); if (!bctx) { LOGP(DBSSGP, LOGL_ERROR, "Failed to create BSSGP context\n"); - nsvc = NULL; + the_pcu.nsvc = NULL; gprs_ns_destroy(bssgp_nsi); bssgp_nsi = NULL; return -EINVAL; @@ -643,7 +651,7 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip,
void gprs_bssgp_destroy_or_exit(void) { - if (exit_on_destroy) { + if (the_pcu.exit_on_destroy) { LOGP(DBSSGP, LOGL_NOTICE, "Exiting on BSSGP destruction.\n"); exit(0); } @@ -655,7 +663,7 @@ void gprs_bssgp_destroy_or_exit(void)
osmo_signal_unregister_handler(SS_L_NS, nsvc_signal_cb, NULL);
- nsvc = NULL; + the_pcu.nsvc = NULL;
/* FIXME: move this to libgb: btsctx_free() */ llist_del(&bctx->list); @@ -671,5 +679,5 @@ void gprs_bssgp_destroy_or_exit(void) void gprs_bssgp_exit_on_destroy(void) { LOGP(DBSSGP, LOGL_NOTICE, "Going to quit on BSSGP destruction\n"); - exit_on_destroy = 1; + the_pcu.exit_on_destroy = 1; }
From: Holger Hans Peter Freyther holger@moiji-mobile.com
--- src/gprs_bssgp_pcu.cpp | 47 ++++++++++++++++++++++++++--------------------- src/gprs_bssgp_pcu.h | 4 ++-- src/gprs_rlcmac.cpp | 1 + src/gprs_rlcmac_data.cpp | 3 ++- 4 files changed, 31 insertions(+), 24 deletions(-)
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp index e94b5eb..2b5068e 100644 --- a/src/gprs_bssgp_pcu.cpp +++ b/src/gprs_bssgp_pcu.cpp @@ -23,6 +23,7 @@
struct osmo_pcu { struct gprs_nsvc *nsvc; + struct bssgp_bvc_ctx *bctx;
int bvc_sig_reset; int bvc_reset; @@ -34,7 +35,6 @@ static struct osmo_pcu the_pcu = { 0, };
struct sgsn_instance *sgsn; extern void *tall_pcu_ctx; -struct bssgp_bvc_ctx *bctx = NULL; extern uint16_t spoof_mcc, spoof_mnc;
struct osmo_timer_list bvc_timer; @@ -538,12 +538,12 @@ static int nsvc_signal_cb(unsigned int subsys, unsigned int signal,
int gprs_bssgp_tx_fc_bvc(void) { - if (!bctx) { + if (!the_pcu.bctx) { LOGP(DBSSGP, LOGL_ERROR, "No bctx\n"); return -EIO; } /* FIXME: use real values */ - return bssgp_tx_fc_bvc(bctx, 1, 6553500, 819100, 50000, 50000, + return bssgp_tx_fc_bvc(the_pcu.bctx, 1, 6553500, 819100, 50000, 50000, NULL, NULL); // return bssgp_tx_fc_bvc(bctx, 1, 84000, 25000, 48000, 45000, // NULL, NULL); @@ -555,29 +555,29 @@ static void bvc_timeout(void *_priv)
if (!the_pcu.bvc_sig_reset) { LOGP(DBSSGP, LOGL_INFO, "Sending reset on BVCI 0\n"); - bssgp_tx_bvc_reset(bctx, 0, BSSGP_CAUSE_OML_INTERV); + bssgp_tx_bvc_reset(the_pcu.bctx, 0, BSSGP_CAUSE_OML_INTERV); osmo_timer_schedule(&bvc_timer, 1, 0); return; }
if (!the_pcu.bvc_reset) { LOGP(DBSSGP, LOGL_INFO, "Sending reset on BVCI %d\n", - bctx->bvci); - bssgp_tx_bvc_reset(bctx, bctx->bvci, BSSGP_CAUSE_OML_INTERV); + the_pcu.bctx->bvci); + bssgp_tx_bvc_reset(the_pcu.bctx, the_pcu.bctx->bvci, BSSGP_CAUSE_OML_INTERV); osmo_timer_schedule(&bvc_timer, 1, 0); return; }
if (!the_pcu.bvc_unblocked) { LOGP(DBSSGP, LOGL_INFO, "Sending unblock on BVCI %d\n", - bctx->bvci); - bssgp_tx_bvc_unblock(bctx); + the_pcu.bctx->bvci); + bssgp_tx_bvc_unblock(the_pcu.bctx); osmo_timer_schedule(&bvc_timer, 1, 0); return; }
LOGP(DBSSGP, LOGL_DEBUG, "Sending flow control info on BVCI %d\n", - bctx->bvci); + the_pcu.bctx->bvci); gprs_bssgp_tx_fc_bvc(); osmo_timer_schedule(&bvc_timer, bts->fc_interval, 0); } @@ -595,7 +595,7 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip, mnc = ((mnc & 0xf00) >> 8) * 100 + ((mnc & 0x0f0) >> 4) * 10 + (mnc & 0x00f); cell_id = ntohs(cell_id);
- if (bctx) + if (the_pcu.bctx) return 0; /* if already created, must return 0: no error */
bssgp_nsi = gprs_ns_instantiate(&sgsn_ns_cb, tall_pcu_ctx); @@ -625,23 +625,23 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip, return -EINVAL; }
- bctx = btsctx_alloc(bvci, nsei); - if (!bctx) { + the_pcu.bctx = btsctx_alloc(bvci, nsei); + if (!the_pcu.bctx) { LOGP(DBSSGP, LOGL_ERROR, "Failed to create BSSGP context\n"); the_pcu.nsvc = NULL; gprs_ns_destroy(bssgp_nsi); bssgp_nsi = NULL; return -EINVAL; } - bctx->ra_id.mcc = spoof_mcc ? : mcc; - bctx->ra_id.mnc = spoof_mnc ? : mnc; - bctx->ra_id.lac = lac; - bctx->ra_id.rac = rac; - bctx->cell_id = cell_id; + the_pcu.bctx->ra_id.mcc = spoof_mcc ? : mcc; + the_pcu.bctx->ra_id.mnc = spoof_mnc ? : mnc; + the_pcu.bctx->ra_id.lac = lac; + the_pcu.bctx->ra_id.rac = rac; + the_pcu.bctx->cell_id = cell_id;
osmo_signal_register_handler(SS_L_NS, nsvc_signal_cb, NULL);
-// bssgp_tx_bvc_reset(bctx, bctx->bvci, BSSGP_CAUSE_PROTO_ERR_UNSPEC); +// bssgp_tx_bvc_reset(the_pcu.bctx, the_pcu.bctx->bvci, BSSGP_CAUSE_PROTO_ERR_UNSPEC);
bvc_timer.cb = bvc_timeout;
@@ -666,9 +666,9 @@ void gprs_bssgp_destroy_or_exit(void) the_pcu.nsvc = NULL;
/* FIXME: move this to libgb: btsctx_free() */ - llist_del(&bctx->list); - talloc_free(bctx); - bctx = NULL; + llist_del(&the_pcu.bctx->list); + talloc_free(the_pcu.bctx); + the_pcu.bctx = NULL;
/* FIXME: blocking... */
@@ -681,3 +681,8 @@ void gprs_bssgp_exit_on_destroy(void) LOGP(DBSSGP, LOGL_NOTICE, "Going to quit on BSSGP destruction\n"); the_pcu.exit_on_destroy = 1; } + +struct bssgp_bvc_ctx *gprs_bssgp_pcu_current_bctx(void) +{ + return the_pcu.bctx; +} diff --git a/src/gprs_bssgp_pcu.h b/src/gprs_bssgp_pcu.h index dc57e4f..dff86a3 100644 --- a/src/gprs_bssgp_pcu.h +++ b/src/gprs_bssgp_pcu.h @@ -41,8 +41,6 @@ struct bssgp_bvc_ctx *btsctx_alloc(uint16_t bvci, uint16_t nsei); #define NS_HDR_LEN 4 #define IE_LLC_PDU 14
-extern struct bssgp_bvc_ctx *bctx; - int gprs_bssgp_pcu_rx_dl_ud(struct msgb *msg, struct tlv_parsed *tp);
int gprs_bssgp_pcu_rx_ptp(struct msgb *msg, struct tlv_parsed *tp, struct bssgp_bvc_ctx *bctx); @@ -59,4 +57,6 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip, uint16_t void gprs_bssgp_exit_on_destroy(void); void gprs_bssgp_destroy_or_exit(void);
+struct bssgp_bvc_ctx *gprs_bssgp_pcu_current_bctx(void); + #endif // GPRS_BSSGP_PCU_H diff --git a/src/gprs_rlcmac.cpp b/src/gprs_rlcmac.cpp index 2ea0a61..f3a24f7 100644 --- a/src/gprs_rlcmac.cpp +++ b/src/gprs_rlcmac.cpp @@ -1789,6 +1789,7 @@ int gprs_rlcmac_tx_ul_ud(gprs_rlcmac_tbf *tbf) uint8_t qos_profile[3]; struct msgb *llc_pdu; unsigned msg_len = NS_HDR_LEN + BSSGP_HDR_LEN + tbf->llc_index; + struct bssgp_bvc_ctx *bctx = gprs_bssgp_pcu_current_bctx();
LOGP(DBSSGP, LOGL_INFO, "LLC [PCU -> SGSN] TFI: %u TLLI: 0x%08x len=%d\n", tbf->tfi, tbf->tlli, tbf->llc_index); if (!bctx) { diff --git a/src/gprs_rlcmac_data.cpp b/src/gprs_rlcmac_data.cpp index 4f2b649..99fac1b 100644 --- a/src/gprs_rlcmac_data.cpp +++ b/src/gprs_rlcmac_data.cpp @@ -1253,7 +1253,8 @@ static struct msgb *llc_dequeue(struct gprs_rlcmac_tbf *tbf) frames = 0xff; if (octets > 0xffffff) octets = 0xffffff; - bssgp_tx_llc_discarded(bctx, tbf->tlli, frames, octets); + bssgp_tx_llc_discarded(gprs_bssgp_pcu_current_bctx(), + tbf->tlli, frames, octets); }
return msg;
From: Holger Hans Peter Freyther holger@moiji-mobile.com
--- src/gprs_bssgp_pcu.cpp | 1 - 1 file changed, 1 deletion(-)
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp index 2b5068e..66c4cb7 100644 --- a/src/gprs_bssgp_pcu.cpp +++ b/src/gprs_bssgp_pcu.cpp @@ -33,7 +33,6 @@ struct osmo_pcu {
static struct osmo_pcu the_pcu = { 0, };
-struct sgsn_instance *sgsn; extern void *tall_pcu_ctx; extern uint16_t spoof_mcc, spoof_mnc;
From: Holger Hans Peter Freyther holger@moiji-mobile.com
This continues with the previous changes to reduce the global state. --- src/gprs_bssgp_pcu.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp index 66c4cb7..8c68bf6 100644 --- a/src/gprs_bssgp_pcu.cpp +++ b/src/gprs_bssgp_pcu.cpp @@ -25,6 +25,8 @@ struct osmo_pcu { struct gprs_nsvc *nsvc; struct bssgp_bvc_ctx *bctx;
+ struct osmo_timer_list bvc_timer; + int bvc_sig_reset; int bvc_reset; int bvc_unblocked; @@ -36,8 +38,6 @@ static struct osmo_pcu the_pcu = { 0, }; extern void *tall_pcu_ctx; extern uint16_t spoof_mcc, spoof_mnc;
-struct osmo_timer_list bvc_timer; - static void bvc_timeout(void *_priv);
static int parse_imsi(struct tlv_parsed *tp, char *imsi) @@ -523,7 +523,7 @@ static int nsvc_signal_cb(unsigned int subsys, unsigned int signal, case S_NS_BLOCK: if (nsvc_unblocked) { nsvc_unblocked = 0; - osmo_timer_del(&bvc_timer); + osmo_timer_del(&the_pcu.bvc_timer); the_pcu.bvc_sig_reset = 0; the_pcu.bvc_reset = 0; the_pcu.bvc_unblocked = 0; @@ -555,7 +555,7 @@ static void bvc_timeout(void *_priv) if (!the_pcu.bvc_sig_reset) { LOGP(DBSSGP, LOGL_INFO, "Sending reset on BVCI 0\n"); bssgp_tx_bvc_reset(the_pcu.bctx, 0, BSSGP_CAUSE_OML_INTERV); - osmo_timer_schedule(&bvc_timer, 1, 0); + osmo_timer_schedule(&the_pcu.bvc_timer, 1, 0); return; }
@@ -563,7 +563,7 @@ static void bvc_timeout(void *_priv) LOGP(DBSSGP, LOGL_INFO, "Sending reset on BVCI %d\n", the_pcu.bctx->bvci); bssgp_tx_bvc_reset(the_pcu.bctx, the_pcu.bctx->bvci, BSSGP_CAUSE_OML_INTERV); - osmo_timer_schedule(&bvc_timer, 1, 0); + osmo_timer_schedule(&the_pcu.bvc_timer, 1, 0); return; }
@@ -571,14 +571,14 @@ static void bvc_timeout(void *_priv) LOGP(DBSSGP, LOGL_INFO, "Sending unblock on BVCI %d\n", the_pcu.bctx->bvci); bssgp_tx_bvc_unblock(the_pcu.bctx); - osmo_timer_schedule(&bvc_timer, 1, 0); + osmo_timer_schedule(&the_pcu.bvc_timer, 1, 0); return; }
LOGP(DBSSGP, LOGL_DEBUG, "Sending flow control info on BVCI %d\n", the_pcu.bctx->bvci); gprs_bssgp_tx_fc_bvc(); - osmo_timer_schedule(&bvc_timer, bts->fc_interval, 0); + osmo_timer_schedule(&the_pcu.bvc_timer, bts->fc_interval, 0); }
/* create BSSGP/NS layer instances */ @@ -642,7 +642,7 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip,
// bssgp_tx_bvc_reset(the_pcu.bctx, the_pcu.bctx->bvci, BSSGP_CAUSE_PROTO_ERR_UNSPEC);
- bvc_timer.cb = bvc_timeout; + the_pcu.bvc_timer.cb = bvc_timeout;
return 0; @@ -658,7 +658,7 @@ void gprs_bssgp_destroy_or_exit(void) if (!bssgp_nsi) return;
- osmo_timer_del(&bvc_timer); + osmo_timer_del(&the_pcu.bvc_timer);
osmo_signal_unregister_handler(SS_L_NS, nsvc_signal_cb, NULL);
From: Holger Hans Peter Freyther holger@moiji-mobile.com
--- src/gprs_bssgp_pcu.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp index 8c68bf6..edb088c 100644 --- a/src/gprs_bssgp_pcu.cpp +++ b/src/gprs_bssgp_pcu.cpp @@ -27,6 +27,8 @@ struct osmo_pcu {
struct osmo_timer_list bvc_timer;
+ int nsvc_unblocked; + int bvc_sig_reset; int bvc_reset; int bvc_unblocked; @@ -492,7 +494,6 @@ static int sgsn_ns_cb(enum gprs_ns_evt event, struct gprs_nsvc *nsvc, struct msg return rc; }
-static int nsvc_unblocked = 0;
static int nsvc_signal_cb(unsigned int subsys, unsigned int signal, void *handler_data, void *signal_data) @@ -510,8 +511,8 @@ static int nsvc_signal_cb(unsigned int subsys, unsigned int signal,
switch (signal) { case S_NS_UNBLOCK: - if (!nsvc_unblocked) { - nsvc_unblocked = 1; + if (!the_pcu.nsvc_unblocked) { + the_pcu.nsvc_unblocked = 1; LOGP(DPCU, LOGL_NOTICE, "NS-VC %d is unblocked.\n", the_pcu.nsvc->nsvci); the_pcu.bvc_sig_reset = 0; @@ -521,8 +522,8 @@ static int nsvc_signal_cb(unsigned int subsys, unsigned int signal, } break; case S_NS_BLOCK: - if (nsvc_unblocked) { - nsvc_unblocked = 0; + if (the_pcu.nsvc_unblocked) { + the_pcu.nsvc_unblocked = 0; osmo_timer_del(&the_pcu.bvc_timer); the_pcu.bvc_sig_reset = 0; the_pcu.bvc_reset = 0;
From: Holger Hans Peter Freyther holger@moiji-mobile.com
This might explain the issue of the BVCI > 1 not being unblocked as the internal state has not been re-set when destroying the bssgp. --- src/gprs_bssgp_pcu.cpp | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp index edb088c..2e93a02 100644 --- a/src/gprs_bssgp_pcu.cpp +++ b/src/gprs_bssgp_pcu.cpp @@ -671,6 +671,10 @@ void gprs_bssgp_destroy_or_exit(void) the_pcu.bctx = NULL;
/* FIXME: blocking... */ + the_pcu.nsvc_unblocked = 0; + the_pcu.bvc_sig_reset = 0; + the_pcu.bvc_reset = 0; + the_pcu.bvc_unblocked = 0;
gprs_ns_destroy(bssgp_nsi); bssgp_nsi = NULL;
From: Holger Hans Peter Freyther holger@moiji-mobile.com
--- src/gprs_bssgp_pcu.cpp | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp index 2e93a02..b90f2c5 100644 --- a/src/gprs_bssgp_pcu.cpp +++ b/src/gprs_bssgp_pcu.cpp @@ -545,8 +545,6 @@ int gprs_bssgp_tx_fc_bvc(void) /* FIXME: use real values */ return bssgp_tx_fc_bvc(the_pcu.bctx, 1, 6553500, 819100, 50000, 50000, NULL, NULL); -// return bssgp_tx_fc_bvc(bctx, 1, 84000, 25000, 48000, 45000, -// NULL, NULL); }
static void bvc_timeout(void *_priv) @@ -641,8 +639,6 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip,
osmo_signal_register_handler(SS_L_NS, nsvc_signal_cb, NULL);
-// bssgp_tx_bvc_reset(the_pcu.bctx, the_pcu.bctx->bvci, BSSGP_CAUSE_PROTO_ERR_UNSPEC); - the_pcu.bvc_timer.cb = bvc_timeout;
From: Holger Hans Peter Freyther holger@moiji-mobile.com
Decrease the number of lines of a single method by splitting things up. The fewer lines of code, branches and side-effects in a method, the easier it will be to understand. The other benefit is that one can start creating unit tests for the some parts of the code. --- src/gprs_bssgp_pcu.cpp | 75 +++++++++++++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 32 deletions(-)
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp index b90f2c5..e994f9f 100644 --- a/src/gprs_bssgp_pcu.cpp +++ b/src/gprs_bssgp_pcu.cpp @@ -71,6 +71,47 @@ static int parse_imsi(struct tlv_parsed *tp, char *imsi) return 0; }
+static int parse_ra_cap_ms_class(struct tlv_parsed *tp) +{ + bitvec *block; + unsigned rp = 0; + uint8_t ms_class = 0; + uint8_t cap_len; + uint8_t *cap; + + if (!TLVP_PRESENT(tp, BSSGP_IE_MS_RADIO_ACCESS_CAP)) + return ms_class; + + cap_len = TLVP_LEN(tp, BSSGP_IE_MS_RADIO_ACCESS_CAP); + cap = (uint8_t *) TLVP_VAL(tp, BSSGP_IE_MS_RADIO_ACCESS_CAP); + + block = bitvec_alloc(cap_len); + bitvec_unpack(block, cap); + bitvec_read_field(block, rp, 4); // Access Technology Type + bitvec_read_field(block, rp, 7); // Length of Access Capabilities + bitvec_read_field(block, rp, 3); // RF Power Capability + if (bitvec_read_field(block, rp, 1)) // A5 Bits Present + bitvec_read_field(block, rp, 7); // A5 Bits + bitvec_read_field(block, rp, 1); // ES IND + bitvec_read_field(block, rp, 1); // PS + bitvec_read_field(block, rp, 1); // VGCS + bitvec_read_field(block, rp, 1); // VBS + if (bitvec_read_field(block, rp, 1)) { // Multislot Cap Present + if (bitvec_read_field(block, rp, 1)) // HSCSD Present + bitvec_read_field(block, rp, 5); // Class + if (bitvec_read_field(block, rp, 1)) { // GPRS Present + ms_class = bitvec_read_field(block, rp, 5); // Class + bitvec_read_field(block, rp, 1); // Ext. + } + if (bitvec_read_field(block, rp, 1)) // SMS Present + bitvec_read_field(block, rp, 4); // SMS Value + bitvec_read_field(block, rp, 4); // SMS Value + } + + bitvec_free(block); + return ms_class; +} + int gprs_bssgp_pcu_rx_dl_ud(struct msgb *msg, struct tlv_parsed *tp) { struct bssgp_ud_hdr *budh; @@ -107,38 +148,8 @@ int gprs_bssgp_pcu_rx_dl_ud(struct msgb *msg, struct tlv_parsed *tp) parse_imsi(tp, imsi);
/* parse ms radio access capability */ - uint8_t ms_class = 0; - if (TLVP_PRESENT(tp, BSSGP_IE_MS_RADIO_ACCESS_CAP)) - { - bitvec *block; - uint8_t cap_len = TLVP_LEN(tp, BSSGP_IE_MS_RADIO_ACCESS_CAP); - uint8_t *cap = (uint8_t *) TLVP_VAL(tp, BSSGP_IE_MS_RADIO_ACCESS_CAP); - unsigned rp = 0; - - block = bitvec_alloc(cap_len); - bitvec_unpack(block, cap); - bitvec_read_field(block, rp, 4); // Access Technology Type - bitvec_read_field(block, rp, 7); // Length of Access Capabilities - bitvec_read_field(block, rp, 3); // RF Power Capability - if (bitvec_read_field(block, rp, 1)) // A5 Bits Present - bitvec_read_field(block, rp, 7); // A5 Bits - bitvec_read_field(block, rp, 1); // ES IND - bitvec_read_field(block, rp, 1); // PS - bitvec_read_field(block, rp, 1); // VGCS - bitvec_read_field(block, rp, 1); // VBS - if (bitvec_read_field(block, rp, 1)) { // Multislot Cap Present - if (bitvec_read_field(block, rp, 1)) // HSCSD Present - bitvec_read_field(block, rp, 5); // Class - if (bitvec_read_field(block, rp, 1)) { // GPRS Present - ms_class = bitvec_read_field(block, rp, 5); // Class - bitvec_read_field(block, rp, 1); // Ext. - } - if (bitvec_read_field(block, rp, 1)) // SMS Present - bitvec_read_field(block, rp, 4); // SMS Value - bitvec_read_field(block, rp, 4); // SMS Value - } - bitvec_free(block); - } + uint8_t ms_class = parse_ra_cap_ms_class(tp); + /* get lifetime */ uint16_t delay_csec = 0xffff; if (TLVP_PRESENT(tp, BSSGP_IE_PDU_LIFETIME))
On Sat, Jul 27, 2013 at 10:20:47PM +0200, Holger Freyther wrote:
Dear Ivan,
I have 14 more patches after this patchset. Could you please indicate how we can end up with a mode where my commit queue is draining within a reasonable amount of time? I started with the PCU Emu to re-produce some SGSN issue(s).
I really appreciate someone reviewing and arguing about the changes I do and each individual one is fairly small and one topic at a time.
Any ideas?
kind regards holger
Hi Holger,
I looked through your patches and commited them to master, all of them are reasonable. I have only one question about the name of the this variable: +static struct osmo_pcu the_pcu = { 0, }; Why do you use "the_pcu", not "pcu"? Actually it is not the problem, just unusual for me. :)
Now I am ready for new patchset. What is the reasonable amount of time for patchset review for you? Sometimes I am overloaded by other work, but I will try to delay review no longer than 1-2 days. Another mode, you can commit patches to master by yourself and just notify about it mailing list.
2013/7/29 Holger Hans Peter Freyther hfreyther@sysmocom.de
On Sat, Jul 27, 2013 at 10:20:47PM +0200, Holger Freyther wrote:
Dear Ivan,
I have 14 more patches after this patchset. Could you please indicate how we can end up with a mode where my commit queue is draining within a reasonable amount of time? I started with the PCU Emu to re-produce some SGSN issue(s).
I really appreciate someone reviewing and arguing about the changes I do and each individual one is fairly small and one topic at a time.
Any ideas?
kind regards 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, Jul 30, 2013 at 12:56:59PM +0400, Ivan Kluchnikov wrote:
Hi Holger,
Dear Ivan,
thanks a lot.
+static struct osmo_pcu the_pcu = { 0, }; Why do you use "the_pcu", not "pcu"?
I wanted to differentiate that 'pcu' is not a local variable. I could have picked g_pcu or something else. I used 'the' because there is only one pcu right now. I will remove that limit though (and remove global state from the PCU).
Now I am ready for new patchset.
Thanks. I will send things tomorrow.
What is the reasonable amount of time for patchset review for you? Sometimes I am overloaded by other work, but I will try to delay review no longer than 1-2 days.
'reasonable' anything < 5 days. I just want to avoid ending with 40 patches and you as the maintainer not having time to look at them. E.g. for OpenBSC I try to at least give a quick NACK/or ask for changes.
Another mode, you can commit patches to master by yourself and just notify about it mailing list.
I would love to have more eyes before the commit hits the repository. Everybody is making mistakes and it would be nice if mine are catched with review too.
cheers holger
2013/7/30 Holger Hans Peter Freyther hfreyther@sysmocom.de
I wanted to differentiate that 'pcu' is not a local variable. I could have picked g_pcu or something else. I used 'the' because there is only one pcu right now. I will remove that limit though (and remove global state from the PCU).
Ok, I understand.
Now I am ready for new patchset.
Thanks. I will send things tomorrow.
What is the reasonable amount of time for patchset review for you? Sometimes I am overloaded by other work, but I will try to delay review
no
longer than 1-2 days.
'reasonable' anything < 5 days. I just want to avoid ending with 40 patches and you as the maintainer not having time to look at them. E.g. for OpenBSC I try to at least give a quick NACK/or ask for changes.
I think we should use this mode, because as you mentioned, reviews are the good opportunity to catch mistakes.
osmocom-net-gprs@lists.osmocom.org
-
Holger Freyther -
Holger Hans Peter Freyther
-
Ivan Kluchnikov