From: Pablo Neira Ayuso pablo@netfilter.org
Hi!
The following patchset contains one fix and two improvements. You can find them in the pablo/updates branch.
Please, merge it!
Pablo Neira Ayuso (3): msc: bail out if subscriber VTY command fails abis: skip e1_input nesting if empty bsc: on-demand setup of nanoBTS and HSL femto sockets
openbsc/src/libabis/e1_input_vty.c | 8 ++++++-- openbsc/src/libbsc/bsc_init.c | 20 +++++++++++++++----- openbsc/src/libmsc/vty_interface_layer3.c | 11 ++++++++++- 3 files changed, 31 insertions(+), 8 deletions(-)
From: Pablo Neira Ayuso pablo@gnumonks.org
This patch adds several messages that would be displayed if:
* the Ki argument is missing. * you pass an invalid Ki. * the database fails to perform the operation (add/delete/update).
Before this patch, no messages were spotted on this errors.
I noticed this while adding Ki to the existing subscribers in the nanoBTS setup: I introduced a wrong Ki but the VTY command line did not report any error. A quick look at the database via sqlite command confirmed that the new authkey information was not added. --- openbsc/src/libmsc/vty_interface_layer3.c | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/openbsc/src/libmsc/vty_interface_layer3.c b/openbsc/src/libmsc/vty_interface_layer3.c index 2d3dd14..6ac2c65 100644 --- a/openbsc/src/libmsc/vty_interface_layer3.c +++ b/openbsc/src/libmsc/vty_interface_layer3.c @@ -575,6 +575,8 @@ DEFUN(ena_subscr_a3a8, } else { /* Unknown method */ subscr_put(subscr); + vty_out(vty, "%% Unknown auth method %s%s", + alg_str, VTY_NEWLINE); return CMD_WARNING; }
@@ -582,6 +584,8 @@ DEFUN(ena_subscr_a3a8, rc = hexparse(ki_str, ainfo.a3a8_ki, sizeof(ainfo.a3a8_ki)); if ((rc > maxlen) || (rc < minlen)) { subscr_put(subscr); + vty_out(vty, "%% Wrong Ki `%s'%s", + ki_str, VTY_NEWLINE); return CMD_WARNING; } ainfo.a3a8_ki_len = rc; @@ -589,6 +593,7 @@ DEFUN(ena_subscr_a3a8, ainfo.a3a8_ki_len = 0; if (minlen) { subscr_put(subscr); + vty_out(vty, "%% Missing Ki argument%s", VTY_NEWLINE); return CMD_WARNING; } } @@ -601,7 +606,11 @@ DEFUN(ena_subscr_a3a8, db_sync_lastauthtuple_for_subscr(NULL, subscr); subscr_put(subscr);
- return rc ? CMD_WARNING : CMD_SUCCESS; + if (rc) { + vty_out(vty, "%% Operation has failed%s", VTY_NEWLINE); + return CMD_WARNING; + } + return CMD_SUCCESS; }
DEFUN(subscriber_purge,
On Tue, May 03, 2011 at 10:44:39PM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
This patch adds several messages that would be displayed if:
thanks, applied.
From: Pablo Neira Ayuso pablo@gnumonks.org
With this patch, we don't including e1_input if it's empty
[...] timeslot 7 phys_chan_config TCH/F hopping enabled 0 e1_input <----------------- empty, it should not show up. msc [...] --- openbsc/src/libabis/e1_input_vty.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/openbsc/src/libabis/e1_input_vty.c b/openbsc/src/libabis/e1_input_vty.c index 7dbf17a..af984bc 100644 --- a/openbsc/src/libabis/e1_input_vty.c +++ b/openbsc/src/libabis/e1_input_vty.c @@ -19,6 +19,7 @@
#include <stdlib.h> #include <unistd.h> +#include <stdbool.h>
#include <osmocom/vty/command.h> #include <osmocom/vty/buffer.h> @@ -75,10 +76,13 @@ DEFUN(cfg_e1inp, cfg_e1inp_cmd, static int e1inp_config_write(struct vty *vty) { struct e1inp_line *line; - - vty_out(vty, "e1_input%s", VTY_NEWLINE); + bool heading = false;
llist_for_each_entry(line, &e1inp_line_list, list) { + if (!heading) { + vty_out(vty, "e1_input%s", VTY_NEWLINE); + heading = true; + } vty_out(vty, " e1_line %u driver %s%s", line->num, line->driver->name, VTY_NEWLINE); }
On 05/03/2011 10:44 PM, pablo@gnumonks.org wrote:
- vty_out(vty, "e1_input%s", VTY_NEWLINE);
- bool heading = false;
what about
if (llist_empty(&e1in..) return;
llist_for_each_entry(line, &e1inp_line_list, list) {
if (!heading) {vty_out(vty, "e1_input%s", VTY_NEWLINE);heading = true;}
On 03/05/11 23:14, Holger Hans Peter Freyther wrote:
On 05/03/2011 10:44 PM, pablo@gnumonks.org wrote:
- vty_out(vty, "e1_input%s", VTY_NEWLINE);
- bool heading = false;
what about
if (llist_empty(&e1in..) return;
Please, find it attached the new patch with your suggestion.
From: Pablo Neira Ayuso pablo@gnumonks.org
The daemon set up nanoBTS and HSL femto sockets by default, ie. the three sockets to support these two drivers are open even if we have no BTS of that kind. This patch enables them if they are present in any configuration file. --- openbsc/src/libbsc/bsc_init.c | 20 +++++++++++++++----- 1 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/openbsc/src/libbsc/bsc_init.c b/openbsc/src/libbsc/bsc_init.c index 8ea2cfa..09d826e 100644 --- a/openbsc/src/libbsc/bsc_init.c +++ b/openbsc/src/libbsc/bsc_init.c @@ -19,6 +19,7 @@ * */
+#include <stdbool.h> #include <openbsc/gsm_data.h> #include <osmocom/gsm/gsm_utils.h> #include <openbsc/gsm_04_08.h> @@ -415,6 +416,7 @@ int bsc_bootstrap_network(int (*mncc_recv)(struct gsm_network *, struct msgb *), struct telnet_connection dummy_conn; struct gsm_bts *bts; int rc; + bool enabled_nanobts = false, enabled_hsl = false;
/* initialize our data structures */ bsc_gsmnet = gsm_network_init(1, 1, mncc_recv); @@ -444,7 +446,20 @@ int bsc_bootstrap_network(int (*mncc_recv)(struct gsm_network *, struct msgb *),
switch (bts->type) { case GSM_BTS_TYPE_NANOBTS: + if (!enabled_nanobts) { + rc = ipaccess_setup(bsc_gsmnet); + if (rc < 0) + break; + enabled_nanobts = true; + } + break; case GSM_BTS_TYPE_HSL_FEMTO: + if (!enabled_hsl) { + rc = hsl_setup(bsc_gsmnet); + if (rc < 0) + break; + enabled_hsl = true; + } break; default: rc = e1_reconfig_bts(bts); @@ -456,10 +471,5 @@ int bsc_bootstrap_network(int (*mncc_recv)(struct gsm_network *, struct msgb *), exit (1); } } - - /* initialize nanoBTS support omce */ - rc = ipaccess_setup(bsc_gsmnet); - rc = hsl_setup(bsc_gsmnet); - return 0; }
On 05/03/2011 10:44 PM, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
The daemon set up nanoBTS and HSL femto sockets by default, ie. the three sockets to support these two drivers are open even if we have no BTS of that kind. This patch enables them if they are present in any configuration file.
good goal, but maybe trigger this from within the VTY code? E.g. what if I want to add a HSL BTS to a running system? or start with no config file at all? Anyway our story of doing such config changes at runtime is not very good.
holger
On Tue, May 03, 2011 at 11:29:16PM +0200, Holger Hans Peter Freyther wrote:
On 05/03/2011 10:44 PM, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
The daemon set up nanoBTS and HSL femto sockets by default, ie. the three sockets to support these two drivers are open even if we have no BTS of that kind. This patch enables them if they are present in any configuration file.
good goal, but maybe trigger this from within the VTY code? E.g. what if I want to add a HSL BTS to a running system? or start with no config file at all? Anyway our story of doing such config changes at runtime is not very good.
yes, but I agree we should do this from the VTY code. At least all new features we're adding should be more runtime vty based then thinking of a config file that we parse once and then forget about it.
So when we get a 'bts type ...' command (or when we leave the BTS_NODE) we should check if the respective sockets are needed.
But yes, another big long pending TODO is to think about how we can make most or even all runtime config changes work.
On 04/05/11 11:02, Harald Welte wrote:
On Tue, May 03, 2011 at 11:29:16PM +0200, Holger Hans Peter Freyther wrote:
On 05/03/2011 10:44 PM, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
The daemon set up nanoBTS and HSL femto sockets by default, ie. the three sockets to support these two drivers are open even if we have no BTS of that kind. This patch enables them if they are present in any configuration file.
good goal, but maybe trigger this from within the VTY code? E.g. what if I want to add a HSL BTS to a running system? or start with no config file at all? Anyway our story of doing such config changes at runtime is not very good.
yes, but I agree we should do this from the VTY code. At least all new features we're adding should be more runtime vty based then thinking of a config file that we parse once and then forget about it.
So when we get a 'bts type ...' command (or when we leave the BTS_NODE) we should check if the respective sockets are needed.
You can find a patch attached to support what you describe, I tried to make as generic as possible. Please, have a look at the description.
The new start(...) function for e1inp_driver objects could be use to replace the e1inp_init() call. The idea that I propose is to register the dahdi and misdn input drivers if we receive an "e1_input" command (via VTY or config file), then invoke start(...).
BTW, e1inp_init() is only used by osmo-nitb, any reason for that?
I also added a new attribute to the BTS model object, this field is useless for bs11. I found no way to relate e1inp_driver and bts objects in the case of hsl and ipaccess. This is usually done by means of the "e1_input" command in bs11, but its use does not make sense for hsl and ipaccess.
But yes, another big long pending TODO is to think about how we can make most or even all runtime config changes work.
Adding support for config file reload in runtime seems interesting to me, I would prefer changing things in the config file and reloading than typing all those VTY commands to add a new BTS, IMO.
-
Harald Welte -
Holger Hans Peter Freyther -
Pablo Neira Ayuso -
pablo＠gnumonks.org