diff mbox

NAT: allow allocating BSC in arbitrary order

Message ID 1459959646-27455-1-git-send-email-msuraev@sysmocom.de
State Not Applicable
Headers show

Commit Message

Max April 6, 2016, 4:20 p.m. UTC 
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(-)

Comments

Neels Hofmeyr April 6, 2016, 11:04 p.m. UTC | #1 
>  
>  	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
Holger Freyther April 7, 2016, 7:18 a.m. UTC | #2 
> 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
Max April 7, 2016, 10:03 a.m. UTC | #3 
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
>
Neels Hofmeyr April 7, 2016, 10:26 a.m. UTC | #4 
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
diff mbox

Patch

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;