Change in libosmocore[master]: vty: track parent nodes also for telnet sessions

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/.

laforge gerrit-no-reply at lists.osmocom.org
Mon Nov 25 16:07:19 UTC 2019


laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/16162 )

Change subject: vty: track parent nodes also for telnet sessions
......................................................................

vty: track parent nodes also for telnet sessions

Keep track of parent nodes and go back hierarchically, not only for .cfg file
reading, but also for telnet VTY sessions.

A long time ago cfg file parsing was made strictly hierarchical: node exits go
back to parent nodes exactly as they were entered. However, live telnet VTY
sessions still lacked this and depended on the go_parent_cb().

>From this commit on, implementing a go_parent_cb() is completely optional. The
go_parent_cb() no longer has the task to determine the correct parent node,
neither for cfg files (as already the case before this patch) nor for telnet
VTY sessions (added by this patch). Instead, a go_parent_cb() implementation
can merely take actions it requires on node exits, for example applying some
config when leaving a specific node.

The node value that is returned by the go_parent_cb() and the vty->node and
vty->index values that might be set are completely ignored; instead the
implicit parent node tracking determines the parent and node object.

As a side effect, the is_config_node() callback is no longer needed, since the
VTY now always implicitly knows when to exit back to the CONFIG_NODE.

For example, osmo_ss7_is_config_node() could now be dropped, and the
osmo_ss7_vty_go_parent() could be shortened by five switch cases, does no
longer need to set vty->node nor vty->index and could thus be shortened to:

int osmo_ss7_vty_go_parent(struct vty *vty)
{
        struct osmo_ss7_asp *asp;
        struct osmo_xua_server *oxs;

        switch (vty->node) {
        case L_CS7_ASP_NODE:
                asp = vty->index;
                /* If no local addr was set */
                if (!asp->cfg.local.host_cnt) {
                        asp->cfg.local.host[0] = NULL;
                        asp->cfg.local.host_cnt = 1;
                }
                osmo_ss7_asp_restart(asp);
                break;
        case L_CS7_XUA_NODE:
                oxs = vty->index;
                /* If no local addr was set, or erased after _create(): */
                if (!oxs->cfg.local.host_cnt)
                        osmo_ss7_xua_server_set_local_host(oxs, NULL);
                if (osmo_ss7_xua_server_bind(oxs) < 0)
                        vty_out(vty, "%% Unable to bind xUA server to IP(s)%s", VTY_NEWLINE);
                break;
        }
        return 0;
}

Before parent tracking, every program was required to write a go_parent_cb()
which has to return every node's parent node, basically a switch() statement
that manually traces the way back out of child nodes. If the go_parent_cb() has
errors, we may wildly jump around the node tree: a common error is to jump
right out to the top config node with one exit, even though we were N levels
deep. This kind of error has been eliminated for cfg files long ago, but still
exists for telnet VTY sessions, which this patch fixes.

This came up when I was adding multi-level config nodes to osmo-hlr to support
Distributed GSM / remote MS lookup: the config file worked fine, while vty node
tests failed to exit to the correct nodes.

Change-Id: I2b32b4fe20732728db6e9cdac7e484d96ab86dc5
---
M include/osmocom/vty/vty.h
M src/vty/command.c
M tests/vty/vty_test.c
3 files changed, 31 insertions(+), 48 deletions(-)

Approvals:
  Jenkins Builder: Verified
  pespin: Looks good to me, approved
  laforge: Looks good to me, but someone else must approve



diff --git a/include/osmocom/vty/vty.h b/include/osmocom/vty/vty.h
index 03a2924..9acaa7d 100644
--- a/include/osmocom/vty/vty.h
+++ b/include/osmocom/vty/vty.h
@@ -178,9 +178,14 @@
 	const char *copyright;
 	/*! \ref talloc context */
 	void *tall_ctx;
-	/*! call-back for returning to parent n ode */
+	/*! Call-back for taking actions upon exiting a node.
+	 * The return value is ignored, and changes to vty->node and vty->index made in this callback are ignored.
+	 * Implicit parent node tracking always sets the correct parent node and vty->index after this callback exits,
+	 * so this callback can handle only those nodes that should take specific actions upon node exit, or can be left
+	 * NULL entirely. */
 	int (*go_parent_cb)(struct vty *vty);
-	/*! call-back to determine if node is config node */
+	/*! OBSOLETED: Implicit parent node tracking has replaced the use of this callback. This callback is no longer
+	 * called, ever, and can be left NULL. */
 	int (*is_config_node)(struct vty *vty, int node);
 	/*! Check if the config is consistent before write */
 	int (*config_is_consistent)(struct vty *vty);
diff --git a/src/vty/command.c b/src/vty/command.c
index 6a9d18a..daee5c5 100644
--- a/src/vty/command.c
+++ b/src/vty/command.c
@@ -200,18 +200,6 @@
 	return strcmp(a->cmd, b->cmd);
 }
 
-static int is_config_child(struct vty *vty)
-{
-	if (vty->node <= CONFIG_NODE)
-		return 0;
-	else if (vty->node > CONFIG_NODE && vty->node < _LAST_OSMOVTY_NODE)
-		return 1;
-	else if (host.app_info->is_config_node)
-		return host.app_info->is_config_node(vty, vty->node);
-	else
-		return vty->node > CONFIG_NODE;
-}
-
 /*! Sort each node's command element according to command string. */
 void sort_node(void)
 {
@@ -2187,25 +2175,10 @@
 			vty_clear_parents(vty);
 			break;
 
-		case CFG_LOG_NODE:
-		case VTY_NODE:
-			vty->node = CONFIG_NODE;
-			vty_clear_parents(vty);
-			break;
-
 		default:
-			if (host.app_info->go_parent_cb) {
+			if (host.app_info->go_parent_cb)
 				host.app_info->go_parent_cb(vty);
-				vty_pop_parent(vty);
-			}
-			else if (is_config_child(vty)) {
-				vty->node = CONFIG_NODE;
-				vty_clear_parents(vty);
-			}
-			else {
-				vty->node = VIEW_NODE;
-				vty_clear_parents(vty);
-			}
+			vty_pop_parent(vty);
 			break;
 	}
 
@@ -2365,9 +2338,30 @@
 
 	if (matched_element->daemon)
 		rc = CMD_SUCCESS_DAEMON;
-	else	/* Execute matched command. */
+	else {
+		/* Execute matched command. */
+		struct vty_parent_node this_node = {
+				.node = vty->node,
+				.priv = vty->priv,
+				.indent = vty->indent,
+			};
+		struct vty_parent_node *parent = vty_parent(vty);
 		rc = (*matched_element->func) (matched_element, vty, argc, argv);
 
+		/* If we have stepped down into a child node, push a parent frame.
+		 * The causality is such: we don't expect every single node entry implementation to push
+		 * a parent node entry onto vty->parent_nodes. Instead we expect vty_go_parent() to *pop*
+		 * a parent node. Hence if the node changed without the parent node changing, we must
+		 * have stepped into a child node. */
+		if (vty->node != this_node.node && parent == vty_parent(vty)
+		    && vty->node > CONFIG_NODE) {
+			/* Push the parent node. */
+			parent = talloc_zero(vty, struct vty_parent_node);
+			*parent = this_node;
+			llist_add(&parent->entry, &vty->parent_nodes);
+		}
+	}
+
 rc_free_deopt_ctx:
 	/* Now after we called the command func, we can free temporary strings */
 	talloc_free(cmd_deopt_ctx);
diff --git a/tests/vty/vty_test.c b/tests/vty/vty_test.c
index 1139638..9627b6d 100644
--- a/tests/vty/vty_test.c
+++ b/tests/vty/vty_test.c
@@ -482,27 +482,11 @@
 	destroy_test_vty(&test, vty);
 }
 
-static int go_parent_cb(struct vty *vty)
-{
-	/*
-	 * - For the interactive VTY tests above, it is expected to bounce back to
-	 *   the CONFIG_NODE. Hence do so in go_parent_cb().
-	 * - In the config file parsing tests, setting vty->node in go_parent_cb() has no
-	 *   effect, because we will subsequently pop a parent node from the parent stack
-	 *   and override to go to the node that was recorded as the actual parent.
-	 */
-	vty->node = CONFIG_NODE;
-	vty->index = NULL;
-	return 0;
-}
-
 int main(int argc, char **argv)
 {
 	struct vty_app_info vty_info = {
 		.name		= "VtyTest",
 		.version	= 0,
-		.go_parent_cb	= go_parent_cb,
-		.is_config_node	= NULL,
 	};
 
 	const struct log_info_cat default_categories[] = {};

-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/16162
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I2b32b4fe20732728db6e9cdac7e484d96ab86dc5
Gerrit-Change-Number: 16162
Gerrit-PatchSet: 3
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20191125/8d782056/attachment.htm>


More information about the gerrit-log mailing list