Re: [libvirt] [PATCH] virsh: add custom readline generator

On 06/24/2011 12:42 AM, Lai Jiangshan wrote: > Custom readline generator will help for some usecase. Indeed, this could be very nice as extended to more commands. > > Also add a custom readline generator for the "help" command. > > Signed-off-by: Lai Jiangshan <laijs(a)cn.fujitsu.com&gt; > --- Hmm, this is a new virsh feature, and we've missed the RC1 deadline, even though your v1 was posted before then. On one hand, it seems like this would be self-contained enough that I would be okay with including a v2 in 0.9.3, unless anyone else objects in the next day or so. However, there are some cleanups to do first, and in the process of identifying those, I have a suggestion for a complete revamp of the patch idea which would certainly put us into post-release approval: > diff --git a/tools/virsh.c b/tools/virsh.c > index fcd254d..51e43c1 100644 > --- a/tools/virsh.c > +++ b/tools/virsh.c > @@ -13575,7 +13575,7 @@ vshCloseLogFile(vshControl *ctl) > * (i.e. STATE == 0), then we start at the top of the list. > */ > static char * > -vshReadlineCommandGenerator(const char *text, int state) > +vshReadlineCmdAndGrpGenerator(const char *text, int state, int grpname) You are using grpname as a bool, so s/int/bool/ > static char * > +vshReadlineCommandGenerator(const char *text, int state) > +{ > + return vshReadlineCmdAndGrpGenerator(text, state, 0); > +} > + > +static char * > +vshReadlineHelpOptionGenerator(const char *text, int state) > +{ > + return vshReadlineCmdAndGrpGenerator(text, state, 1); and pass false/true rather than 0/1. > +} > + > +struct vshCustomReadLine { > + const char *name; > + char *(*CustomReadLineOptionGenerator)(const char *text, int state); > +}; > + > +struct vshCustomReadLine customeReadLine[] = { > + { "help", vshReadlineHelpOptionGenerator }, > + { NULL, NULL } Rather than maintaining yet another array of mappings, along with the corresponding cost of name lookups within that array, what if we instead change vshCmdDef to instead have a new function pointer field, which is NULL if the default command completion is good enough, but which is the particular command's custom completer when we want a smarter function. > +static struct vshCustomReadLine *vshCustomReadLineSearch(const char *name) > +{ > + struct vshCustomReadLine *ret = customeReadLine; s/custome/custom/, if we even keep this function. But I don't think we need it, because... > @@ -13632,6 +13672,7 @@ vshReadlineOptionsGenerator(const char *text, int state) > memcpy(cmdname, rl_line_buffer, p - rl_line_buffer); > > cmd = vshCmddefSearch(cmdname); > + rl = vshCustomReadLineSearch(cmdname); ...here, you could just get the custom generator as a member of the enlarged vshCmdDef struct. One other idea would be to add new enums to vshCmdOptType. For example, VSH_OT_CMD would be identical to VSH_OT_STRING, except that during tab-completion, it completes on command and group names (for that matter, while VSH_OT_STRING and VSH_OT_DATA used to have separate purposes, I no longer see any difference between them, and we could probably consolidate those two types). But I don't know if that makes tab completion any easier or harder. In terms of your help example, the two ideas play out roughly like: 1. 'help TAB' -> completion generator parses first word, 'help', and sees that it is a command. The help command has a custom generator, so invoke that. The custom generator for help then knows that the only possibility for a next word is a command or group name, and returns the full list of such names. 2. 'help TAB' -> completion generator parses first word, and learns that 'help' command has a VSH_OT_CMD second argument. It then calls the CMD generator, which knows how to generate the full list of such names. But when you get to other commands, you can start to see the benefit of having strongly-typed VSH_OT arguments: 1. 'vol-key --pool TAB' -> completion generator parses first word and sees that it is the 'vol-key' command. It then calls the vol-key command generator, which must parse the second word '--pool' to see that completion wants a name of a pool, rather than a volume id. But there are quite a few vol-* commands, all of which would have to repeat this --pool parsing. 2. 'vol-key --pool TAB' -> completion generator parses '--pool' and sees that it requires an argument, and that it is of type VSH_OT_POOL. It is then able to provide a list of all pool names, merged also with the list of all other possible options (in this case, --vol). That is, if you have command-based custom generators, then each command has to repeat parsing functionality, then call back to common list generators; whereas if you have option-based custom generators, then you have fewer callbacks because all the smarts are tied to well-typed options, and after all, it is the type of each option that determines which list to generate, more than the type of the command that includes the option.