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