From: Max msuraev@sysmocom.de
Check for existing BSC before allocating new one. Track number of remaining BSCs on deallocation. Explicitly use BSC number in allocation function. --- openbsc/include/openbsc/bsc_nat.h | 3 ++- openbsc/src/osmo-bsc_nat/bsc_nat_utils.c | 9 +++++++-- openbsc/src/osmo-bsc_nat/bsc_nat_vty.c | 14 ++++---------- openbsc/tests/bsc-nat/bsc_nat_test.c | 14 +++++++------- 4 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/openbsc/include/openbsc/bsc_nat.h b/openbsc/include/openbsc/bsc_nat.h index 7ede1fb..5ccc02e 100644 --- a/openbsc/include/openbsc/bsc_nat.h +++ b/openbsc/include/openbsc/bsc_nat.h @@ -325,7 +325,8 @@ struct bsc_nat_ussd_con { };
/* create and init the structures */ -struct bsc_config *bsc_config_alloc(struct bsc_nat *nat, const char *token); +struct bsc_config *bsc_config_alloc(struct bsc_nat *nat, const char *token, + unsigned int number); struct bsc_config *bsc_config_num(struct bsc_nat *nat, int num); struct bsc_config *bsc_config_by_token(struct bsc_nat *nat, const char *token, int len); void bsc_config_free(struct bsc_config *); diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat_utils.c b/openbsc/src/osmo-bsc_nat/bsc_nat_utils.c index cc7d442..1bf3266 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_nat_utils.c +++ b/openbsc/src/osmo-bsc_nat/bsc_nat_utils.c @@ -155,14 +155,15 @@ struct bsc_connection *bsc_connection_alloc(struct bsc_nat *nat) return con; }
-struct bsc_config *bsc_config_alloc(struct bsc_nat *nat, const char *token) +struct bsc_config *bsc_config_alloc(struct bsc_nat *nat, const char *token, + unsigned int number) { struct bsc_config *conf = talloc_zero(nat, struct bsc_config); if (!conf) return NULL;
conf->token = talloc_strdup(conf, token); - conf->nr = nat->num_bsc; + conf->nr = number; conf->nat = nat; conf->max_endpoints = 32; conf->paging_group = PAGIN_GROUP_UNASSIGNED; @@ -205,6 +206,10 @@ void bsc_config_free(struct bsc_config *cfg) llist_del(&cfg->entry); rate_ctr_group_free(cfg->stats.ctrg); talloc_free(cfg); + cfg->nat->num_bsc--; + if (cfg->nat->num_bsc < 0) + LOGP(DNAT, LOGL_ERROR, "Internal error while deallocating BSC " + "config: negative BSC index!\n"); }
static void _add_lac(void *ctx, struct llist_head *list, int _lac) diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c b/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c index 26b0fd6..b7b49e6 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c +++ b/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c @@ -837,17 +837,11 @@ DEFUN(cfg_bsc, cfg_bsc_cmd, "bsc BSC_NR", "BSC configuration\n" "Identifier of the BSC\n") { int bsc_nr = atoi(argv[0]); - struct bsc_config *bsc; + struct bsc_config *bsc = bsc_config_num(_nat, bsc_nr);
- if (bsc_nr > _nat->num_bsc) { - vty_out(vty, "%% The next unused BSC number is %u%s", - _nat->num_bsc, VTY_NEWLINE); - return CMD_WARNING; - } else if (bsc_nr == _nat->num_bsc) { - /* allocate a new one */ - bsc = bsc_config_alloc(_nat, "unknown"); - } else - bsc = bsc_config_num(_nat, bsc_nr); + /* allocate a new one */ + if (!bsc) + bsc = bsc_config_alloc(_nat, "unknown", bsc_nr);
if (!bsc) return CMD_WARNING; diff --git a/openbsc/tests/bsc-nat/bsc_nat_test.c b/openbsc/tests/bsc-nat/bsc_nat_test.c index a4b313c..a405763 100644 --- a/openbsc/tests/bsc-nat/bsc_nat_test.c +++ b/openbsc/tests/bsc-nat/bsc_nat_test.c @@ -316,7 +316,7 @@ static void test_contrack() printf("Testing connection tracking.\n"); nat = bsc_nat_alloc(); con = bsc_connection_alloc(nat); - con->cfg = bsc_config_alloc(nat, "foo"); + con->cfg = bsc_config_alloc(nat, "foo", 0); bsc_config_add_lac(con->cfg, 23); bsc_config_add_lac(con->cfg, 49); bsc_config_add_lac(con->cfg, 42); @@ -434,7 +434,7 @@ static void test_paging(void)
nat = bsc_nat_alloc(); con = bsc_connection_alloc(nat); - cfg = bsc_config_alloc(nat, "unknown"); + cfg = bsc_config_alloc(nat, "unknown", 0); con->cfg = cfg; bsc_config_add_lac(cfg, 23); con->authenticated = 1; @@ -476,7 +476,7 @@ static void test_mgcp_allocations(void) nat->mgcp_cfg->trunk.number_endpoints = 64;
bsc = bsc_connection_alloc(nat); - bsc->cfg = bsc_config_alloc(nat, "foo"); + bsc->cfg = bsc_config_alloc(nat, "foo", 0); bsc->cfg->max_endpoints = 60; bsc_config_add_lac(bsc->cfg, 2323); bsc->last_endpoint = 0x22; @@ -522,7 +522,7 @@ static void test_mgcp_ass_tracking(void) mgcp_endpoints_allocate(&nat->mgcp_cfg->trunk);
bsc = bsc_connection_alloc(nat); - bsc->cfg = bsc_config_alloc(nat, "foo"); + bsc->cfg = bsc_config_alloc(nat, "foo", 0); bsc_config_add_lac(bsc->cfg, 2323); bsc->last_endpoint = 0x1e; con.bsc = bsc; @@ -874,7 +874,7 @@ static void test_cr_filter()
struct bsc_nat *nat = bsc_nat_alloc(); struct bsc_connection *bsc = bsc_connection_alloc(nat); - bsc->cfg = bsc_config_alloc(nat, "foo"); + bsc->cfg = bsc_config_alloc(nat, "foo", 0); bsc_config_add_lac(bsc->cfg, 1234); bsc->cfg->acc_lst_name = "bsc"; nat->acc_lst_name = "nat"; @@ -953,7 +953,7 @@ static void test_dt_filter() struct bsc_connection *bsc = bsc_connection_alloc(nat); struct nat_sccp_connection *con = talloc_zero(0, struct nat_sccp_connection);
- bsc->cfg = bsc_config_alloc(nat, "foo"); + bsc->cfg = bsc_config_alloc(nat, "foo", 0); bsc_config_add_lac(bsc->cfg, 23); con->bsc = bsc;
@@ -1525,7 +1525,7 @@ static void test_nat_extract_lac() /* initialize the testcase */ nat = bsc_nat_alloc(); bsc = bsc_connection_alloc(nat); - bsc->cfg = bsc_config_alloc(nat, "foo"); + bsc->cfg = bsc_config_alloc(nat, "foo", 0);
memset(&con, 0, sizeof(con)); con.bsc = bsc;
conf->token = talloc_strdup(conf, token);
- conf->nr = nat->num_bsc;
- conf->nr = number;
I think you could completely remove the num_bsc variable? It looks like its sole use was to determine the next available BSC number without iterating the list.
conf->nat = nat; conf->max_endpoints = 32; conf->paging_group = PAGIN_GROUP_UNASSIGNED; @@ -205,6 +206,10 @@ void bsc_config_free(struct bsc_config *cfg) llist_del(&cfg->entry); rate_ctr_group_free(cfg->stats.ctrg); talloc_free(cfg);
- cfg->nat->num_bsc--;
- if (cfg->nat->num_bsc < 0)
LOGP(DNAT, LOGL_ERROR, "Internal error while deallocating BSC ""config: negative BSC index!\n");}
I don't understand why you would add this check for negative BSC index. The llist_del() should ensure that we don't double free a BSC, right?
Also nice would be to add a test case that has a non-null BSC number, to show that having gaps in the numbering doesn't have side effects.
~Neels
On 07 Apr 2016, at 01:04, Neels Hofmeyr nhofmeyr@sysmocom.de wrote:
conf->token = talloc_strdup(conf, token);
- conf->nr = nat->num_bsc;
- conf->nr = number;
I think you could completely remove the num_bsc variable? It looks like its sole use was to determine the next available BSC number without iterating the list.
I have emotional attachments to this variable and would prefer to keep it around.
- cfg->nat->num_bsc--;
- if (cfg->nat->num_bsc < 0)
LOGP(DNAT, LOGL_ERROR, "Internal error while deallocating BSC ""config: negative BSC index!\n");}
I don't understand why you would add this check for negative BSC index. The llist_del() should ensure that we don't double free a BSC, right?
more like maintaining an invariant. So maybe OSMO_ASSERT(num_bsc >= 0) is better?
Also nice would be to add a test case that has a non-null BSC number, to show that having gaps in the numbering doesn't have side effects.
Do you have an idea for the testcase?
holger
Thanks for review. I can add the test-case for adding bscs with numbers 0, 1, 3 in vty tests for dynamic conf reloading - I'm adding/removing it there anyway.
On 04/07/2016 09:18 AM, Holger Freyther wrote:
On 07 Apr 2016, at 01:04, Neels Hofmeyr nhofmeyr@sysmocom.de wrote: Also nice would be to add a test case that has a non-null BSC number, to show that having gaps in the numbering doesn't have side effects.
Do you have an idea for the testcase?
holger
On Thu, Apr 07, 2016 at 09:18:09AM +0200, Holger Freyther wrote:
Do you have an idea for the testcase?
- make sure I can create BTS 23 first, then BTS 0 and then BTS 5, and find all of them again later; and that all three are cleaned up.
- what happens when I try to create BTS -42.
- what happens when I try to access BTS 7 though it wasn't created.
~Neels