This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Review at https://gerrit.osmocom.org/3880
VTY: implicit node exit by de-indenting, not parent lookup
Note: This will break users' config files if they do not use exactly one space
of indenting per vty child node level.
When reading VTY commands from a file, use indenting as means to implicitly
exit child nodes. Do not look for commands in the parent node implicitly.
The VTY so far implies 'exit' commands if a VTY line cannot be parsed on the
current node, but succeeds on the parent node. That is the mechanism by which
our VTY config files do not need 'exit' at the end of each child node.
We've hit problems with this in the following scenarios, which will show
improved user experience after this patch:
*) When both a parent and its child node have commands with identical names:
cs7 instace 0
point-code 1.2.3
sccp-address osmo-msc
point-code 0.0.1
If I put the parent's command below the child, it is still interpreted in the
context of the child node:
cs7 instace 0
sccp-address osmo-msc
point-code 0.0.1
point-code 1.2.3
Though the indenting lets me assume I am setting the cs7 instance's global PC
to 1.2.3, I'm actually overwriting osmo-msc's PC with 1.2.3 and discarding the
0.0.1.
*) When a software change moves a VTY command from a child to a parent. Say
'timezone' moved from 'bts' to 'network' level:
network
timezone 1 2
Say a user still has an old config file with 'timezone' on the child level:
network
bts 0
timezone 1 2
trx 0
The user would expect an error message that 'timezone' is invalid on the 'bts'
level. Instead, the VTY finds the parent node's 'timezone', steps out of 'bts'
to the 'network' level, and instead says that the 'trx' command does not exist.
Implementation:
Track node depth: whenever a command enters a child node, we increase the depth
counter. If the amount of spaces that indent a following VTY command are less
than this expected depth, call vty_go_parent() until it matches up.
Transitions to child node are spread across VTY implementations and are hard to
change. But transitions to the parent node are all handled by vty_go_parent().
By decreasing a depth counter in vty_go_parent(), we can also detect that a
command has changed the node without decreasing the depth, hence it must have
stepped into a child node, and we can increase the depth.
The behavior on the interactive telnet VTY remains unchanged.
Change-Id: I24cbb3f6de111f2d31110c3c484c066f1153aac9
---
M include/osmocom/vty/command.h
M include/osmocom/vty/vty.h
M src/vty/command.c
M src/vty/vty.c
M tests/Makefile.am
M tests/testsuite.at
A tests/vty/fail_not_de-indented.cfg
A tests/vty/fail_too_much_indent.cfg
A tests/vty/ok.cfg
M tests/vty/vty_test.c
M tests/vty/vty_test.ok
11 files changed, 125 insertions(+), 23 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/80/3880/1
diff --git a/include/osmocom/vty/command.h b/include/osmocom/vty/command.h
index 0fa5175..9817d73 100644
--- a/include/osmocom/vty/command.h
+++ b/include/osmocom/vty/command.h
@@ -161,6 +161,8 @@
#define CMD_COMPLETE_MATCH 8
#define CMD_COMPLETE_LIST_MATCH 9
#define CMD_SUCCESS_DAEMON 10
+#define CMD_ERR_INVALID_INDENT 11
+#define CMD_ERR_TOO_MUCH_INDENT 12
/* Argc max counts. */
#define CMD_ARGC_MAX 256
@@ -368,6 +370,7 @@
char *argv_concat(const char **argv, int argc, int shift);
vector cmd_make_strvec(const char *);
+int cmd_make_strvec2(const char *string, int *indent, vector *strvec_p);
void cmd_free_strvec(vector);
vector cmd_describe_command();
char **cmd_complete_command();
diff --git a/include/osmocom/vty/vty.h b/include/osmocom/vty/vty.h
index 544e1fa..448b343 100644
--- a/include/osmocom/vty/vty.h
+++ b/include/osmocom/vty/vty.h
@@ -134,6 +134,9 @@
/*! In configure mode. */
int config;
+
+ /*! track how deep we descended into child nodes for reading config files */
+ int node_depth;
};
/* Small macro to determine newline is newline only or linefeed needed. */
diff --git a/src/vty/command.c b/src/vty/command.c
index 33862c0..aa5d0d8 100644
--- a/src/vty/command.c
+++ b/src/vty/command.c
@@ -190,31 +190,52 @@
}
}
-/*! Breaking up string into each command piece. I assume given
- character is separated by a space character. Return value is a
- vector which includes char ** data element. */
-vector cmd_make_strvec(const char *string)
+/*! Break up string in command tokens. Count leading indents.
+ * \param[in] string String to split.
+ * \param[out] indent If not NULL, returns leading indents.
+ * \param[out] strvec_p Returns vector of split tokens, must not be NULL.
+ * \returns CMD_SUCCESS or CMD_ERR_INVALID_INDENT
+ *
+ * If \a indent is passed non-NULL, only simple space ' ' indents are allowed,
+ * so that \a indent can simply return the count of leading spaces.
+ * Otherwise any isspace() characters are allowed for indenting (backwards compat).
+ */
+int cmd_make_strvec2(const char *string, int *indent, vector *strvec_p)
{
const char *cp, *start;
char *token;
int strlen;
vector strvec;
+ *strvec_p = NULL;
+ if (indent)
+ *indent = 0;
+
if (string == NULL)
- return NULL;
+ return CMD_SUCCESS;
cp = string;
/* Skip white spaces. */
- while (isspace((int)*cp) && *cp != '\0')
+ while (isspace((int)*cp) && *cp != '\0') {
+ /* if we're counting indents, we need to be strict about them */
+ if (indent && (*cp != ' ')) {
+ /* indicate position of error */
+ *indent = cp - string;
+ return CMD_ERR_INVALID_INDENT;
+ }
cp++;
+ }
+
+ if (indent)
+ *indent = cp - string;
/* Return if there is only white spaces */
if (*cp == '\0')
- return NULL;
+ return CMD_SUCCESS;
if (*cp == '!' || *cp == '#')
- return NULL;
+ return CMD_SUCCESS;
/* Prepare return vector. */
strvec = vector_init(VECTOR_MIN_SIZE);
@@ -236,8 +257,21 @@
cp++;
if (*cp == '\0')
- return strvec;
+ break;
}
+
+ *strvec_p = strvec;
+ return CMD_SUCCESS;
+}
+
+/*! Breaking up string into each command piece. I assume given
+ character is separated by a space character. Return value is a
+ vector which includes char ** data element. */
+vector cmd_make_strvec(const char *string)
+{
+ vector strvec;
+ cmd_make_strvec2(string, NULL, &strvec);
+ return strvec;
}
/*! Free allocated string vector. */
@@ -1969,24 +2003,34 @@
case VIEW_NODE:
case ENABLE_NODE:
case CONFIG_NODE:
+ vty->node_depth = 0;
break;
case AUTH_ENABLE_NODE:
vty->node = VIEW_NODE;
+ vty->node_depth = 0;
break;
case CFG_LOG_NODE:
case VTY_NODE:
vty->node = CONFIG_NODE;
+ vty->node_depth = 0;
break;
default:
- if (host.app_info->go_parent_cb)
+ if (host.app_info->go_parent_cb) {
host.app_info->go_parent_cb(vty);
- else if (is_config_child(vty))
+ if (vty->node_depth > 0)
+ vty->node_depth--;
+ }
+ else if (is_config_child(vty)) {
vty->node = CONFIG_NODE;
- else
+ vty->node_depth = 0;
+ }
+ else {
vty->node = VIEW_NODE;
+ vty->node_depth = 0;
+ }
break;
}
@@ -2281,29 +2325,43 @@
{
int ret;
vector vline;
+ int indent;
+ int prev_node;
+ int prev_node_depth;
+ vty->node_depth = 0;
while (fgets(vty->buf, VTY_BUFSIZ, fp)) {
- vline = cmd_make_strvec(vty->buf);
+ ret = cmd_make_strvec2(vty->buf, &indent, &vline);
- /* In case of comment line */
+ if (ret != CMD_SUCCESS)
+ return CMD_ERR_INVALID_INDENT;
+
+ /* In case of comment or empty 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)) {
+ if (indent > vty->node_depth)
+ return CMD_ERR_TOO_MUCH_INDENT;
+
+ while (indent < vty->node_depth)
vty_go_parent(vty);
- ret = cmd_execute_command_strict(vline, vty, NULL);
- }
+ prev_node = vty->node;
+ prev_node_depth = vty->node_depth;
+ 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;
+
+ /* If we have stepped down into a child node, count the indent.
+ * The causality is such: we don't expect every single node entry implementation to
+ * increment vty->node_depth. Instead we expect vty_go_parent() do *decrement* the
+ * node_depth. Hence if the node changed without node_depth decreasing, we must have
+ * stepped into a child node and now expect a deeper indent. */
+ if (vty->node != prev_node && !(vty->node_depth < prev_node_depth))
+ vty->node_depth++;
}
return CMD_SUCCESS;
}
diff --git a/src/vty/vty.c b/src/vty/vty.c
index 113a781..94ce428 100644
--- a/src/vty/vty.c
+++ b/src/vty/vty.c
@@ -1480,6 +1480,13 @@
case CMD_ERR_NO_MATCH:
fprintf(stderr, "There is no such command.\n");
break;
+ case CMD_ERR_INVALID_INDENT:
+ fprintf(stderr, "Invalid indentation, only space characters allowed.\n");
+ break;
+ case CMD_ERR_TOO_MUCH_INDENT:
+ fprintf(stderr, "Too much indentation, number of leading spaces"
+ " must match the child node depth exactly.\n");
+ break;
}
fprintf(stderr, "Error occurred during reading the below "
"line:\n%s\n", vty->buf);
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 37378fb..8526847 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -217,7 +217,9 @@
logging/logging_test.ok logging/logging_test.err \
fr/fr_test.ok loggingrb/logging_test.ok \
loggingrb/logging_test.err strrb/strrb_test.ok \
- vty/vty_test.ok comp128/comp128_test.ok \
+ vty/vty_test.ok vty/fail_not_de-indented.cfg \
+ vty/fail_too_much_indent.cfg vty/ok.cfg \
+ comp128/comp128_test.ok \
utils/utils_test.ok stats/stats_test.ok \
bitvec/bitvec_test.ok msgb/msgb_test.ok bits/bitcomp_test.ok \
sim/sim_test.ok tlv/tlv_test.ok abis/abis_test.ok \
diff --git a/tests/testsuite.at b/tests/testsuite.at
index f148cf5..1954e66 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -174,6 +174,7 @@
AT_SETUP([vty])
AT_KEYWORDS([vty])
cat $abs_srcdir/vty/vty_test.ok > expout
+cp $abs_srcdir/vty/*.cfg .
AT_CHECK([$abs_top_builddir/tests/vty/vty_test], [0], [expout], [ignore])
AT_CLEANUP
diff --git a/tests/vty/fail_not_de-indented.cfg b/tests/vty/fail_not_de-indented.cfg
new file mode 100644
index 0000000..5af833d
--- /dev/null
+++ b/tests/vty/fail_not_de-indented.cfg
@@ -0,0 +1,3 @@
+line vty
+ no login
+ log stderr
diff --git a/tests/vty/fail_too_much_indent.cfg b/tests/vty/fail_too_much_indent.cfg
new file mode 100644
index 0000000..032af00
--- /dev/null
+++ b/tests/vty/fail_too_much_indent.cfg
@@ -0,0 +1,3 @@
+line vty
+ no login
+log stderr
diff --git a/tests/vty/ok.cfg b/tests/vty/ok.cfg
new file mode 100644
index 0000000..d5ef23e
--- /dev/null
+++ b/tests/vty/ok.cfg
@@ -0,0 +1,3 @@
+line vty
+ no login
+log stderr
diff --git a/tests/vty/vty_test.c b/tests/vty/vty_test.c
index 1e1b495..e24045c 100644
--- a/tests/vty/vty_test.c
+++ b/tests/vty/vty_test.c
@@ -19,6 +19,7 @@
#include <stdio.h>
#include <string.h>
+#include <errno.h>
#include <sys/types.h>
#include <sys/socket.h>
@@ -290,6 +291,15 @@
destroy_test_vty(&test, vty);
}
+void test_exit_by_indent(const char *fname, int expect_rc)
+{
+ int rc;
+ printf("reading file %s, expecting rc=%d\n", fname, expect_rc);
+ rc = vty_read_config_file(fname, NULL);
+ printf("got rc=%d\n", rc);
+ OSMO_ASSERT(rc == expect_rc);
+}
+
int main(int argc, char **argv)
{
struct vty_app_info vty_info = {
@@ -324,6 +334,9 @@
test_cmd_string_from_valstr();
test_node_tree_structure();
test_stats_vty();
+ test_exit_by_indent("ok.cfg", 0);
+ test_exit_by_indent("fail_not_de-indented.cfg", -EINVAL);
+ test_exit_by_indent("fail_too_much_indent.cfg", -EINVAL);
/* Leak check */
OSMO_ASSERT(talloc_total_blocks(stats_ctx) == 1);
diff --git a/tests/vty/vty_test.ok b/tests/vty/vty_test.ok
index a9e283d..00b0853 100644
--- a/tests/vty/vty_test.ok
+++ b/tests/vty/vty_test.ok
@@ -110,4 +110,10 @@
Returned: 0, Current node: 4 '%s(config)# '
Going to execute 'no stats reporter statsd'
Returned: 0, Current node: 4 '%s(config)# '
+reading file ok.cfg, expecting rc=0
+got rc=0
+reading file fail_not_de-indented.cfg, expecting rc=-22
+got rc=-22
+reading file fail_too_much_indent.cfg, expecting rc=-22
+got rc=-22
All tests passed
--
To view, visit https://gerrit.osmocom.org/3880
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I24cbb3f6de111f2d31110c3c484c066f1153aac9
Gerrit-PatchSet: 1
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>