I'd like to share a VTY config error analysis that has a tricky solution:
I started osmo-bsc -c osmo-bsc.cfg with, according to openbsc/doc/examples/osmo-bsc:
net [...] bts 0 [...] periodic location update 30 trx 0 [...]
and get this error:
There is no such command. Error occurred during reading below line: trx 0
what? 'net / bts / trx' is no command??
Solution: I'm on the vlr_3G branch (incorporating 2G via A-interface) and on this branch, I've actually moved the 'periodic location update N' command a level up, from net / bts / periodic to net / periodic (background: assuming that OsmoMSC does not have individual BTS info, we moved some settings up to network level; whether this makes sense for osmo-bsc is a different question, it's just what happens to be on the vlr_3G branch now).
So there is no net / bts / periodic command. Why do I get an error for net / bts / trx instead?
two reasons:
1. 'trx 0' was the line following the 'periodic' command, 2. since 'periodic' exists one level above, the vty code goes to the parent node automatically.
About 2: we tend to indent our VTY config files, but in fact the indentation has no effect whatsoever, it is just eye candy (very useful eye candy).
The code in question: If a command does not exist, try 'vty_go_parent()' and see if the command exists there. That's what allows us to omit 'exit' in our config files to go to the parent node explicitly:
libosmocore/src/vty/command.c:
int config_from_file(struct vty *vty, FILE * fp) { int ret; vector vline;
while (fgets(vty->buf, VTY_BUFSIZ, fp)) { vline = cmd_make_strvec(vty->buf);
/* In case of comment line */ if (vline == NULL) continue; /* Execute configuration command : this is strict match */ ret = cmd_execute_command_strict(vline, vty, NULL);
/* Try again with setting node to CONFIG_NODE */ while (ret != CMD_SUCCESS && ret != CMD_WARNING && ret != CMD_ERR_NOTHING_TODO && is_config_child(vty)) { HERE -----> vty_go_parent(vty); ret = cmd_execute_command_strict(vline, vty, NULL); }
cmd_free_strvec(vline);
if (ret != CMD_SUCCESS && ret != CMD_WARNING && ret != CMD_ERR_NOTHING_TODO) return ret; } return CMD_SUCCESS; }
In this case the 'periodic...' command does in fact now exist one level above, so the vty_go_parent() is successful and running that command works. But, the vty config parsing then sits above on the 'network' level, is no longer in 'bts 0' and hence refuses to accept the following bts-level command, in this case 'trx 0'. Confusing!
So it's not a bug, it's a feature. But it's a feature we might see quite often if we move the 'periodic' command up to network level and users attempt to use their old config files.
Same goes for the 'timezone' command, BTW, so we might want to rename commands, or re-consider moving commands one level up in the first place. Maybe we should leave backward compat catchers in place that print a warning.
~N
Hi Neels,
On Tue, May 09, 2017 at 12:15:12AM +0200, Neels Hofmeyr wrote:
About 2: we tend to indent our VTY config files, but in fact the indentation has no effect whatsoever, it is just eye candy (very useful eye candy).
Maybe we should change that part, rather than renaming commands for no good reason? We could use the indent level to generate 'virtual node exit' commands...
On Tue, May 09, 2017 at 07:06:27AM +0200, Harald Welte wrote:
Hi Neels,
On Tue, May 09, 2017 at 12:15:12AM +0200, Neels Hofmeyr wrote:
About 2: we tend to indent our VTY config files, but in fact the indentation has no effect whatsoever, it is just eye candy (very useful eye candy).
Maybe we should change that part, rather than renaming commands for no good reason? We could use the indent level to generate 'virtual node exit' commands...
And call it "vtython" from python :)
I indeed would prefer that from the implicit exiting. Hoping that most users would anyway have properly indented config files...
On the telnet vty, we can so far also issue commands that belong to the parent and end up on the parent level implicitly, which we would also stop with this change; I think that'd be a good thing.
The code would be slightly non-trivial, simply counting the number of spaces leading each line, and issue an exit for every decrease is not enough: we need to count the number of de-indentings to exit multiple times e.g. at the end of net/bts/trx. I guess we should count tabs as single indents, but make sure that a config file has only tabs or only spaces, not a mixture? count tabs as eight? disallow tabs altogether? Otherwise we could look at how python solves that and maybe copy their code over to C... which would probably be quite complex.
~N
On Tue, May 09, 2017 at 12:46:38PM +0200, Neels Hofmeyr wrote:
On Tue, May 09, 2017 at 07:06:27AM +0200, Harald Welte wrote:
Hi Neels,
On Tue, May 09, 2017 at 12:15:12AM +0200, Neels Hofmeyr wrote:
About 2: we tend to indent our VTY config files, but in fact the indentation has no effect whatsoever, it is just eye candy (very useful eye candy).
Maybe we should change that part, rather than renaming commands for no good reason? We could use the indent level to generate 'virtual node exit' commands...
A patch for this exists on gerrit now and sees some interesting discussion: https://gerrit.osmocom.org/3880
Maybe we can move the discussion here for easier reading / writing: (copying the comments so far from gerrit)
msuraev:
I think it deserves wider discussion in ML because it have a chance to break pretty much every Osmocom config file out there.
I fully agree, and there already was some discussion on the ML, which lead up to this patch: see thread before and after https://lists.osmocom.org/pipermail/openbsc/2017-May/010652.html ("catch-22 VTY error")
I am touching this now because I think we're releasing a burst of people needing to adjust their vty config files these days (with osmo-msc/bsc separation and osmo-ggsn, the new SIGTRAN and whatnot), and it would make sense to do this indenting change at the same time.
Actually I think it might be worth it to make this indenting strictness optional, so that e.g. the old osmo-nitb can still request the old implicit-go-to-parent way, while new binaries from a given version on can switch to the new strict indenting.
Though we're writing a lot here now, I would welcome more discussion on the ML...
hwelte:
Thanks for looking into this. I think it might be difficult to insist on one space idnent per level. It would be much mor user friendly if there was a notion of 'parent node depth', i.e. you can use any number of additional spaces to indent a new depth level, and as soon as you go back beyond that depthy level we go to the parent. Not sure how comple that would be to implement?
Currently the VTY only remembers a single node. It relies on the node commands to pick a child node, and on go_parent_cb() to pick the right parent for each child.
i.e. there is no state about parent nodes at all. To remember which depths each parent node was on, we would need to create such state per parent node. So the effort is non-trivial.
But nevertheless, I like that, because we will hardly ever step in deeper than say five child node levels and keeping the state is far from performance / memory critical. Also it would obsolete the need for the go_parent callbacks, and we could actually define the child->parent relations merely from entering a child. (One step further could even define the parent->child relations statically when defining VTY commands, instead of writing code to enter another node, opening up the possibility to validate that the parent/child structure is sound).
My main question is, how much of this code do we want to change? Is there a consideration like with llist, that we don't want to deviate from upstream too much?
Also, if we now have a notion of the "depth", could this somehow be used to automatically generate the required spaces in front of a string when saving the file (config_write_...())? So far we manually have to print those spaces which is a bit ugly (and doesn't permit a node to appear at different parent nodes / depths).
That's a bit harder, because for writing the config we simply vty_out(). There would need to be some wrapper function prepending indent, plus some function indicating that we stepped in and out of a child node to modify the prepended indent. We still have to take care that we indicate that properly:
network bts 0 arfcn 123 bts 1
From 'arfcn' up to bts 1, we still have to manually indicate that we intended to step out of the bts 0 child node.
Also structures like this are valid:
network bts 0 bts 1
i.e. bts 0 would intend to enter a child node, but bts 1 follows right away on the same level, i.e. steps out of it again. There isn't any way that saves us from indicating those levels manually; we can only save ourselves from varying spaces while in a specific node by accident. Not sure if it's worth the trouble / easier to just indicate the depth by manual spaces.
(and doesn't permit a node to appear at different parent nodes / depths).
For <thing I don't remember> I once wrote a vty_write wrapper that accepted the parent node and level of indent flexibly, so that two different binaries could hook a library vty on arbitrary node depth levels... pretty straightforward.
(end of quote)
~N
On Thu, Sep 07, 2017 at 10:05:54PM +0200, Neels Hofmeyr wrote:
I am touching this now because I think we're releasing a burst of people needing to adjust their vty config files these days
I like how this reads ambiguously :) You get what I mean, right...
~N
-
Harald Welte -
Neels Hofmeyr