[cmaster-next] VTYSH behavior change and extract.pl elimination
Folks, I've been doing some hacking on vtysh in an effort to clean it up and fix some obscure bugs and less-than-sane interactions with daemons. Initially my efforts were focused on getting vtysh to automatically update its vty->node when a daemon node changes instead of needing a manual DEFUN override for every such daemon command. I have this working; after executing a command, the daemon side of the vty returns what node it is in in addition to the command status code and any output, which vtysh uses to update its own node. This in contrast to attempting to synchronize shared state over the life of a vtysh session without actually communicating said state. That said, I think vtysh could use some more work on a slightly larger scale. In my opinion extract.pl is fragile and error-prone. For example, any command definitions outside the source files it looks in (such as the default commands installed by lib/command.c) are not picked up and thus need to be manually replicated in vtysh. Additionally anytime new submodes are added, one has to remember to update extract.pl. And there are quite a few edge cases that vtysh has to handle that ain't pretty. I propose to change vtysh's behavior such that it no longer needs compile-time knowledge of daemon CLI. Instead it should ask each daemon for its CLI at runtime whenever needed, for example when a user types '?' or hits tab. This has several benefits: - extract.pl can go away since vtysh no longer needs to know about daemon cli, and just implements CLI specific to itself - vtysh binary becomes ~93% smaller - problem of keeping vtysh in sync with daemons becomes trivial - daemons gain the ability to choose what CLI they expose - saves confusion caused by vtysh silently accepting CLI for which the appropriate daemon is not running - CLI for daemons which are not running is not shown How to deal with this last point is probably the most important thing to discuss because that is the change the user will see. Since vtysh will no longer have foreknowledge of all CLI, it won't be able to distinguish between a bogus command and a command that's valid but for which the daemon is not running. I think this is an easy one to solve; we can just change % Unknown command to something like, for example, % Unknown command (or daemon not running). As mentioned above I see this as a benefit since it will prevent users getting confused by vtysh silently accepting CLI for stopped daemons. Feedback welcome. Quentin
Even though I asked Quentin to send this email, I'd like to put a +1 on it. Let's think about modifying vtysh to ask running daemons what their command set is. donald On Thu, Dec 8, 2016 at 8:32 PM, Quentin Young <qlyoung@cumulusnetworks.com> wrote:
Folks,
I've been doing some hacking on vtysh in an effort to clean it up and fix some obscure bugs and less-than-sane interactions with daemons. Initially my efforts were focused on getting vtysh to automatically update its vty->node when a daemon node changes instead of needing a manual DEFUN override for every such daemon command. I have this working; after executing a command, the daemon side of the vty returns what node it is in in addition to the command status code and any output, which vtysh uses to update its own node. This in contrast to attempting to synchronize shared state over the life of a vtysh session without actually communicating said state.
That said, I think vtysh could use some more work on a slightly larger scale. In my opinion extract.pl is fragile and error-prone. For example, any command definitions outside the source files it looks in (such as the default commands installed by lib/command.c) are not picked up and thus need to be manually replicated in vtysh. Additionally anytime new submodes are added, one has to remember to update extract.pl. And there are quite a few edge cases that vtysh has to handle that ain't pretty.
I propose to change vtysh's behavior such that it no longer needs compile-time knowledge of daemon CLI. Instead it should ask each daemon for its CLI at runtime whenever needed, for example when a user types '?' or hits tab. This has several benefits:
extract.pl can go away since vtysh no longer needs to know about daemon cli, and just implements CLI specific to itself vtysh binary becomes ~93% smaller problem of keeping vtysh in sync with daemons becomes trivial daemons gain the ability to choose what CLI they expose saves confusion caused by vtysh silently accepting CLI for which the appropriate daemon is not running CLI for daemons which are not running is not shown
How to deal with this last point is probably the most important thing to discuss because that is the change the user will see. Since vtysh will no longer have foreknowledge of all CLI, it won't be able to distinguish between a bogus command and a command that's valid but for which the daemon is not running. I think this is an easy one to solve; we can just change
% Unknown command
to something like, for example,
% Unknown command (or daemon not running).
As mentioned above I see this as a benefit since it will prevent users getting confused by vtysh silently accepting CLI for stopped daemons.
Feedback welcome.
Quentin
_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next
I agree that this is needed, and I have an angle of approach: for this to work, vtysh first needs to have better handling of "node position" in the CLI. With the qobj infrastructure in place (check "dev/osr/vty_index" in git), it's now safe to give out the index pointers to vtysh and not store them in the daemons. This means we can extend the vtysh protocol to return not only success/error, but additionally "new location in VTY". This can be passed back in by vtysh so the daemon side is then stateless. This would work like this: [input] user: "router ospf" [vtysh->zebra] (CONFIG_NODE, obj: 0, "router ospf") [vtysh->ospfd] (CONFIG_NODE, obj: 0, "router ospf") [zebra->vtysh] (CMD_NO_MATCH, CONFIG_NODE, obj:0, obj_identity: "config") [ospfd->vtysh] (CMD_OK, OSPF_NODE, obj:12345678, obj_identity: "router ospf") [output] "router ospf #" vtysh flags zebra as "detached", only ospfd gets commands [input] user: "exit" [vtysh->ospfd] (OSPF_NODE, obj: 12345678, "exit") [ospfd->vtysh] (CMD_OK, CONFIG_NODE, obj: 0, obj_identity: "config") vtysh detects ospfd has moved back to config and "reattaches" zebra to receive commands. Note "exit" could also be handled client-side in vtysh, by just popping off the stack. [input] user: "route-map foobar permit 10" [vtysh->zebra] (CONFIG_NODE, obj: 0, "route-map foobar") [vtysh->ospfd] (CONFIG_NODE, obj: 0, "route-map foobar") [zebra->vtysh] (CMD_OK, RMAP_NODE, obj:45678901, obj_identity: "route-map foobar permit 10") [ospfd->vtysh] (CMD_OK, RMAP_NODE, obj:12345678, obj_identity: "route-map foobar permit 10") [output] "route-map #" Note the object ID is different between zebra and ospfd; qobj ID values are local to the daemons and random. Hoever, obj_identity provides a string value that describes the position *for the specific purpose of equality tests*. This way, vtysh can detect that both daemons are at the same position. ID values are purely opaque and must not be mangled or compared with ID values from another daemon. This is IMHO the neccessary first step since this then vtysh can get rid of all the copied special commands that change the node position, which in turn allows it to download and hold a list of (node, cmd string) without needing to know anything regarding its effects. NB: at the same time I would like to change CONFIG_NODE, RMAP_NODE, etc. to be string values instead of int/enum. The intent there is to get rid of the centralised enum in lib/ that needs to be updated, instead we can use something hierarchical like "ospfd/router" or "bgpd/router/afi_ipv4". Now the really hard problem on this is - I'd like to keep some compatibility for mismatching versions of vtysh <> daemons. On the daemon side it's easy since we can just enable the protocol with a hidden command sent by vtysh first thing on startup. On the vtysh side... not so much, the logic is completely different. But the vtysh side is the important one, if someone updates Quagga and then vtysh can't talk to anything anymore, that's a huge mess... (It's already a problem now if commands get changed, but that hasn't really been an issue so far...) Cheers, -David On Fri, Dec 09, 2016 at 07:44:54AM -0500, Donald Sharp wrote:
Even though I asked Quentin to send this email, I'd like to put a +1 on it. Let's think about modifying vtysh to ask running daemons what their command set is.
donald
On Thu, Dec 8, 2016 at 8:32 PM, Quentin Young <qlyoung@cumulusnetworks.com> wrote:
Folks,
I've been doing some hacking on vtysh in an effort to clean it up and fix some obscure bugs and less-than-sane interactions with daemons. Initially my efforts were focused on getting vtysh to automatically update its vty->node when a daemon node changes instead of needing a manual DEFUN override for every such daemon command. I have this working; after executing a command, the daemon side of the vty returns what node it is in in addition to the command status code and any output, which vtysh uses to update its own node. This in contrast to attempting to synchronize shared state over the life of a vtysh session without actually communicating said state.
That said, I think vtysh could use some more work on a slightly larger scale. In my opinion extract.pl is fragile and error-prone. For example, any command definitions outside the source files it looks in (such as the default commands installed by lib/command.c) are not picked up and thus need to be manually replicated in vtysh. Additionally anytime new submodes are added, one has to remember to update extract.pl. And there are quite a few edge cases that vtysh has to handle that ain't pretty.
I propose to change vtysh's behavior such that it no longer needs compile-time knowledge of daemon CLI. Instead it should ask each daemon for its CLI at runtime whenever needed, for example when a user types '?' or hits tab. This has several benefits:
extract.pl can go away since vtysh no longer needs to know about daemon cli, and just implements CLI specific to itself vtysh binary becomes ~93% smaller problem of keeping vtysh in sync with daemons becomes trivial daemons gain the ability to choose what CLI they expose saves confusion caused by vtysh silently accepting CLI for which the appropriate daemon is not running CLI for daemons which are not running is not shown
How to deal with this last point is probably the most important thing to discuss because that is the change the user will see. Since vtysh will no longer have foreknowledge of all CLI, it won't be able to distinguish between a bogus command and a command that's valid but for which the daemon is not running. I think this is an easy one to solve; we can just change
% Unknown command
to something like, for example,
% Unknown command (or daemon not running).
As mentioned above I see this as a benefit since it will prevent users getting confused by vtysh silently accepting CLI for stopped daemons.
Feedback welcome.
Quentin
_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next
_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next
participants (3)
-
David Lamparter -
Donald Sharp -
Quentin Young