[cmaster-next] VTYSH behavior change and extract.pl elimination

[David Lamparter]

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 at 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 at lists.nox.tf
> > https://lists.nox.tf/listinfo/cmaster-next
> >
> 
> _______________________________________________
> cmaster-next mailing list
> cmaster-next at lists.nox.tf
> https://lists.nox.tf/listinfo/cmaster-next