Change in libosmocore[master]: vty: track parent nodes also for telnet sessions
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
Change in libosmocore[master]: vty: track parent nodes also for telnet sessions
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/16162 ) Change subject: vty: track parent nodes also for telnet sessions .. Patch Set 3: Code-Review+2 -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Comment-Date: Mon, 25 Nov 2019 12:23:20 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in libosmocore[master]: vty: track parent nodes also for telnet sessions
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/16162 ) Change subject: vty: track parent nodes also for telnet sessions .. Patch Set 3: (1 comment) https://gerrit.osmocom.org/c/libosmocore/+/16162/2/include/osmocom/vty/vty.h File include/osmocom/vty/vty.h: https://gerrit.osmocom.org/c/libosmocore/+/16162/2/include/osmocom/vty/vty.h@189 PS2, Line 189: int (*is_config_node)(struct vty *vty, int node); > worth checking if a field in a struct can be marked as deprecated. this compiles fine: int (*is_config_node)(struct vty *vty, int node) OSMO_DEPRECATED("Implicit parent node tracking has replaced the use of this callback. This callback is no longer called, ever, and can be left NULL."); but seems to have no effect: even though osmo-hlr's hlr.c sets an .is_config_node, the compilation doesn't show any deprecation warning. We can decide in the subsequent patch whether we also want to add the OSMO_DEPRECATED warning https://gerrit.osmocom.org/c/libosmocore/+/16188 -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-CC: pespin Gerrit-Comment-Date: Sun, 24 Nov 2019 19:02:42 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin Gerrit-MessageType: comment
Change in libosmocore[master]: vty: track parent nodes also for telnet sessions
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/16162 ) Change subject: vty: track parent nodes also for telnet sessions .. Patch Set 2: (1 comment) https://gerrit.osmocom.org/c/libosmocore/+/16162/2/src/vty/command.c File src/vty/command.c: https://gerrit.osmocom.org/c/libosmocore/+/16162/2/src/vty/command.c@2361 PS2, Line 2361: llist_add(>entry, >parent_nodes); > Don't you need to initialize ->entry beforehand? or having it set to 0 is > fine? INIT_LLIST_HEAD() is only needed for a list's head, not for the entries added to it. -- 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: 2 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-CC: pespin Gerrit-Comment-Date: Sun, 24 Nov 2019 18:43:34 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin Gerrit-MessageType: comment
Change in libosmocore[master]: vty: track parent nodes also for telnet sessions
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/16162 ) Change subject: vty: track parent nodes also for telnet sessions .. Patch Set 2: (2 comments) https://gerrit.osmocom.org/c/libosmocore/+/16162/2/include/osmocom/vty/vty.h File include/osmocom/vty/vty.h: https://gerrit.osmocom.org/c/libosmocore/+/16162/2/include/osmocom/vty/vty.h@189 PS2, Line 189: int (*is_config_node)(struct vty *vty, int node); worth checking if a field in a struct can be marked as deprecated. https://gerrit.osmocom.org/c/libosmocore/+/16162/2/src/vty/command.c File src/vty/command.c: https://gerrit.osmocom.org/c/libosmocore/+/16162/2/src/vty/command.c@2361 PS2, Line 2361: llist_add(>entry, >parent_nodes); Don't you need to initialize ->entry beforehand? or having it set to 0 is fine? -- 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: 2 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-CC: pespin Gerrit-Comment-Date: Fri, 22 Nov 2019 14:44:37 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in libosmocore[master]: vty: track parent nodes also for telnet sessions
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/16162 ) Change subject: vty: track parent nodes also for telnet sessions .. Patch Set 2: Code-Review+1 -- 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: 2 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Comment-Date: Fri, 22 Nov 2019 12:32:03 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in libosmocore[master]: vty: track parent nodes also for telnet sessions
neels has uploaded this change for review. ( 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(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/62/16162/1 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);