--- openbsc/src/libbsc/bsc_ctrl_commands.c | 31 +++++++++++++++++++++++++++++++ openbsc/tests/ctrl_test_runner.py | 18 ++++++++++++++++++ 2 files changed, 49 insertions(+)
diff --git a/openbsc/src/libbsc/bsc_ctrl_commands.c b/openbsc/src/libbsc/bsc_ctrl_commands.c index 3759593..3cb77cc 100644 --- a/openbsc/src/libbsc/bsc_ctrl_commands.c +++ b/openbsc/src/libbsc/bsc_ctrl_commands.c @@ -66,6 +66,36 @@ CTRL_CMD_DEFINE_RANGE(net_mcc, "mcc", struct gsm_network, country_code, 1, 999); CTRL_CMD_VTY_STRING(net_short_name, "short-name", struct gsm_network, name_short); CTRL_CMD_VTY_STRING(net_long_name, "long-name", struct gsm_network, name_long);
+static int verify_net_auth_policy(struct ctrl_cmd *cmd, const char *value, void *data) +{ + + if ((int)gsm_auth_policy_parse(value) < 0) { + return -1; + } + + return 0; +} + +static int get_net_auth_policy(struct ctrl_cmd *cmd, void *data) +{ + struct gsm_network *net = cmd->node; + cmd->reply = talloc_asprintf(cmd, "%s", gsm_auth_policy_name(net->auth_policy)); + if (!cmd->reply) { + cmd->reply = "OOM"; + return CTRL_CMD_ERROR; + } + return CTRL_CMD_REPLY; +} + +static int set_net_auth_policy(struct ctrl_cmd *cmd, void *data) +{ + struct gsm_network *net = cmd->node; + net->auth_policy = gsm_auth_policy_parse(cmd->value); + return get_net_auth_policy(cmd, data); +} + +CTRL_CMD_DEFINE(net_auth_policy, "auth-policy"); + static int verify_net_apply_config(struct ctrl_cmd *cmd, const char *v, void *d) { return 0; @@ -200,6 +230,7 @@ int bsc_base_ctrl_cmds_install(void) rc |= ctrl_cmd_install(CTRL_NODE_ROOT, &cmd_net_mcc); rc |= ctrl_cmd_install(CTRL_NODE_ROOT, &cmd_net_short_name); rc |= ctrl_cmd_install(CTRL_NODE_ROOT, &cmd_net_long_name); + rc |= ctrl_cmd_install(CTRL_NODE_ROOT, &cmd_net_auth_policy); rc |= ctrl_cmd_install(CTRL_NODE_ROOT, &cmd_net_apply_config); rc |= ctrl_cmd_install(CTRL_NODE_ROOT, &cmd_net_mcc_mnc_apply);
diff --git a/openbsc/tests/ctrl_test_runner.py b/openbsc/tests/ctrl_test_runner.py index b50e93c..f24d52a 100644 --- a/openbsc/tests/ctrl_test_runner.py +++ b/openbsc/tests/ctrl_test_runner.py @@ -362,6 +362,24 @@ class TestCtrlNITB(TestCtrlBase): def ctrl_app(self): return (4249, "./src/osmo-nitb/osmo-nitb", "OsmoBSC", "nitb")
+ def testAuthPolicy(self): + policies = ['token', 'closed', 'accept-all'] + + for policy in policies: + r = self.do_set('auth-policy', policy) + self.assertEquals(r['mtype'], 'SET_REPLY') + self.assertEquals(r['var'], 'auth-policy') + self.assertEquals(r['value'], policy) + + r = self.do_get('auth-policy') + self.assertEquals(r['mtype'], 'GET_REPLY') + self.assertEquals(r['var'], 'auth-policy') + self.assertEquals(r['value'], policy) + + r = self.do_set('auth-policy', 'qwerty') + self.assertEquals(r['mtype'], 'ERROR') + self.assertEquals(r['error'], 'Value failed verification.') + def testSubscriberAddRemove(self): r = self.do_set('subscriber-modify-v1', '2620345,445566') self.assertEquals(r['mtype'], 'SET_REPLY')
--- openbsc/src/libbsc/bsc_ctrl_commands.c | 33 ++++++++++++++++++++++++++++++++ openbsc/tests/ctrl_test_runner.py | 19 ++++++++++++++++++ 2 files changed, 52 insertions(+)
diff --git a/openbsc/src/libbsc/bsc_ctrl_commands.c b/openbsc/src/libbsc/bsc_ctrl_commands.c index 3cb77cc..bbd4fbd 100644 --- a/openbsc/src/libbsc/bsc_ctrl_commands.c +++ b/openbsc/src/libbsc/bsc_ctrl_commands.c @@ -183,6 +183,37 @@ oom: } CTRL_CMD_DEFINE(net_mcc_mnc_apply, "mcc-mnc-apply");
+/* BTS related commands below here */ +static int verify_bts_band(struct ctrl_cmd *cmd, const char *value, void *data) +{ + + if ((int)gsm_band_parse(value) < 0) { + return -1; + } + + return 0; +} + +static int get_bts_band(struct ctrl_cmd *cmd, void *data) +{ + struct gsm_bts *bts = cmd->node; + cmd->reply = talloc_asprintf(cmd, "%s", gsm_band_name(bts->band)); + if (!cmd->reply) { + cmd->reply = "OOM"; + return CTRL_CMD_ERROR; + } + return CTRL_CMD_REPLY; +} + +static int set_bts_band(struct ctrl_cmd *cmd, void *data) +{ + struct gsm_bts *bts = cmd->node; + bts->band = gsm_band_parse(cmd->value); + return get_bts_band(cmd, data); +} + +CTRL_CMD_DEFINE(bts_band, "band"); + /* TRX related commands below here */ CTRL_HELPER_GET_INT(trx_max_power, struct gsm_bts_trx, max_power_red); static int verify_trx_max_power(struct ctrl_cmd *cmd, const char *value, void *_data) @@ -234,6 +265,8 @@ int bsc_base_ctrl_cmds_install(void) rc |= ctrl_cmd_install(CTRL_NODE_ROOT, &cmd_net_apply_config); rc |= ctrl_cmd_install(CTRL_NODE_ROOT, &cmd_net_mcc_mnc_apply);
+ rc |= ctrl_cmd_install(CTRL_NODE_BTS, &cmd_bts_band); + rc |= ctrl_cmd_install(CTRL_NODE_TRX, &cmd_trx_max_power); return rc; } diff --git a/openbsc/tests/ctrl_test_runner.py b/openbsc/tests/ctrl_test_runner.py index f24d52a..243ac0f 100644 --- a/openbsc/tests/ctrl_test_runner.py +++ b/openbsc/tests/ctrl_test_runner.py @@ -380,6 +380,25 @@ class TestCtrlNITB(TestCtrlBase): self.assertEquals(r['mtype'], 'ERROR') self.assertEquals(r['error'], 'Value failed verification.')
+ def testBtsBand(self): + bands = ['GSM450', 'GSM480', 'GSM750', 'GSM810', + 'GSM850', 'GSM900', 'DCS1800', 'PCS1900'] + + for band in bands: + r = self.do_set('bts.0.band', band) + self.assertEquals(r['mtype'], 'SET_REPLY') + self.assertEquals(r['var'], 'bts.0.band') + self.assertEquals(r['value'], band) + + r = self.do_get('bts.0.band') + self.assertEquals(r['mtype'], 'GET_REPLY') + self.assertEquals(r['var'], 'bts.0.band') + self.assertEquals(r['value'], band) + + r = self.do_set('bts.0.band', 'qwerty') + self.assertEquals(r['mtype'], 'ERROR') + self.assertEquals(r['error'], 'Value failed verification.') + def testSubscriberAddRemove(self): r = self.do_set('subscriber-modify-v1', '2620345,445566') self.assertEquals(r['mtype'], 'SET_REPLY')
On Tue, May 06, 2014 at 07:35:58PM +0400, Ivan Kluchnikov wrote:
- if ((int)gsm_band_parse(value) < 0) {
return -1;- }
Same as with the previous patch. Either add a bottom element to to the gsm_band or return int from the method. But the above is the wrong thing to do.
- def testBtsBand(self):
bands = ['GSM450', 'GSM480', 'GSM750', 'GSM810','GSM850', 'GSM900', 'DCS1800', 'PCS1900']
We handle PCS1800 or DCS900 as well. Maybe just make it 'public' and add these to the test. E.g. a matrix out of {'GSM','DCS', 'PCS'}x{450, 480, ...} for the test
--- openbsc/src/libbsc/bsc_ctrl_commands.c | 2 ++ openbsc/tests/ctrl_test_runner.py | 15 +++++++++++++++ 2 files changed, 17 insertions(+)
diff --git a/openbsc/src/libbsc/bsc_ctrl_commands.c b/openbsc/src/libbsc/bsc_ctrl_commands.c index bbd4fbd..d1edf29 100644 --- a/openbsc/src/libbsc/bsc_ctrl_commands.c +++ b/openbsc/src/libbsc/bsc_ctrl_commands.c @@ -253,6 +253,7 @@ static int set_trx_max_power(struct ctrl_cmd *cmd, void *_data) return get_trx_max_power(cmd, _data); } CTRL_CMD_DEFINE(trx_max_power, "max-power-reduction"); +CTRL_CMD_DEFINE_RANGE(trx_arfcn, "arfcn", struct gsm_bts_trx, arfcn, 0, 1023);
int bsc_base_ctrl_cmds_install(void) { @@ -268,5 +269,6 @@ int bsc_base_ctrl_cmds_install(void) rc |= ctrl_cmd_install(CTRL_NODE_BTS, &cmd_bts_band);
rc |= ctrl_cmd_install(CTRL_NODE_TRX, &cmd_trx_max_power); + rc |= ctrl_cmd_install(CTRL_NODE_TRX, &cmd_trx_arfcn); return rc; } diff --git a/openbsc/tests/ctrl_test_runner.py b/openbsc/tests/ctrl_test_runner.py index 243ac0f..0dc9800 100644 --- a/openbsc/tests/ctrl_test_runner.py +++ b/openbsc/tests/ctrl_test_runner.py @@ -399,6 +399,21 @@ class TestCtrlNITB(TestCtrlBase): self.assertEquals(r['mtype'], 'ERROR') self.assertEquals(r['error'], 'Value failed verification.')
+ def testTrxArfcn(self): + r = self.do_set('bts.0.trx.0.arfcn', '51') + self.assertEquals(r['mtype'], 'SET_REPLY') + self.assertEquals(r['var'], 'bts.0.trx.0.arfcn') + self.assertEquals(r['value'], '51') + + r = self.do_get('bts.0.trx.0.arfcn') + self.assertEquals(r['mtype'], 'GET_REPLY') + self.assertEquals(r['var'], 'bts.0.trx.0.arfcn') + self.assertEquals(r['value'], '51') + + r = self.do_set('bts.0.trx.0.arfcn', '3000') + self.assertEquals(r['mtype'], 'ERROR') + self.assertEquals(r['error'], 'Input not within the range') + def testSubscriberAddRemove(self): r = self.do_set('subscriber-modify-v1', '2620345,445566') self.assertEquals(r['mtype'], 'SET_REPLY')
On Tue, May 06, 2014 at 07:35:59PM +0400, Ivan Kluchnikov wrote:
Hi again,
+CTRL_CMD_DEFINE_RANGE(trx_arfcn, "arfcn", struct gsm_bts_trx, arfcn, 0, 1023);
Please look at cfg_trx_arfcn_cmd in src/libbsc/bsc_vty.c. There are plenty of "FIXME" in the code and they all apply here. There are two separate issues that your patch is touching.
1.) Align VTY and CTRL command. My approach so far was to move the "handling" into a common method that is used by CTRL/VTY.
2.) The general question of "when" to apply the configuration. For some commands it is immediate, for some comments it is after a BTS is bootstrapped again and for some it requires a re-start of the NITB application.
3.) Make it impossible to configure a "broken" system. For your code and the VTY command it is possible to create and later save/write a config that will not be parsed on re-start.
So this patch at least needs to do:
* Make the CTRL and VTY share a routine to "set" the ARFCN * Move the FIXMEs into this routine * Verify that the BAND + ARFCN would be compatible (so we would still have three FIXMEs left).
cheers holger
On Tue, May 06, 2014 at 07:35:57PM +0400, Ivan Kluchnikov wrote:
hi,
- if ((int)gsm_auth_policy_parse(value) < 0) {
return -1;- }
1.) Coding-Style. 2.) The (int) cast is fishy. Most likely even undefined C usage. We have a int(-EINVAL) -> enum -> int conversion here. Add an invalid element to the enum, change the implementation to compare to -EINVAL...
Then your verify call becomes a simple one liner and comparison against the invalid element.
- def testAuthPolicy(self):
r = self.do_set('auth-policy', 'qwerty')self.assertEquals(r['mtype'], 'ERROR')self.assertEquals(r['error'], 'Value failed verification.')
Check that the previous thing has not been changed. :)
-
Holger Hans Peter Freyther -
Ivan Kluchnikov