[libvirt] [PATCH] virsh: add custom readline generator
Custom readline generator will help for some usecase. Also add a custom readline generator for the "help" command. Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- 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) { static int grp_list_index, cmd_list_index, len; const char *name; @@ -13604,8 +13604,13 @@ vshReadlineCommandGenerator(const char *text, int state) return vshStrdup(NULL, name); } } else { + name = grp[grp_list_index].keyword; cmd_list_index = 0; grp_list_index++; + + if (grpname && STREQLEN(name, text, len)) + return vshStrdup(NULL, name); + } } @@ -13614,10 +13619,45 @@ vshReadlineCommandGenerator(const char *text, int state) } 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); +} + +struct vshCustomReadLine { + const char *name; + char *(*CustomReadLineOptionGenerator)(const char *text, int state); +}; + +struct vshCustomReadLine customeReadLine[] = { + { "help", vshReadlineHelpOptionGenerator }, + { NULL, NULL } +}; + +static struct vshCustomReadLine *vshCustomReadLineSearch(const char *name) +{ + struct vshCustomReadLine *ret = customeReadLine; + + for (ret = customeReadLine; ret->name; ret++) { + if (STREQ(ret->name, name)) + return ret; + } + + return NULL; +} + +static char * vshReadlineOptionsGenerator(const char *text, int state) { static int list_index, len; static const vshCmdDef *cmd = NULL; + static const struct vshCustomReadLine *rl = NULL; const char *name; if (!state) { @@ -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); list_index = 0; len = strlen(text); VIR_FREE(cmdname); @@ -13640,6 +13681,9 @@ vshReadlineOptionsGenerator(const char *text, int state) if (!cmd) return NULL; + if (rl) + return rl->CustomReadLineOptionGenerator(text, state); + if (!cmd->opts) return NULL;
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@cn.fujitsu.com> ---
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. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 27.06.2011 19:37, Eric Blake wrote:
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@cn.fujitsu.com> ---
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.
I was thinking about same feature and started to work on it during this weekend. But I ran into a problem. Basically, what I intended to implement, is: 1.) expand struct vshCmdDef for a char *(*callback) (const char *text, int state). But for real benefit, we need connection object, so we could e.g. list only inactive networks for 'net-start' command. And this is the problem, because we would have to make this object global (since readline functions does not allow passing any opaque value). 2.) expand each command definition with its own completer. So e.g. for start commands we could only list inactive domains, networks, pools, whatever. For destroy only active objects. And so on. Michal
On 06/27/2011 12:06 PM, Michal Privoznik wrote:
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.
I was thinking about same feature and started to work on it during this weekend. But I ran into a problem. Basically, what I intended to implement, is:
1.) expand struct vshCmdDef for a char *(*callback) (const char *text, int state). But for real benefit, we need connection object, so we could e.g. list only inactive networks for 'net-start' command. And this is the problem, because we would have to make this object global (since readline functions does not allow passing any opaque value).
As gross as it is, a global object isn't entirely bad - virsh is single-threaded. And even if we want to make virsh threaded at some point, I still think we can get away with adding a thread-local access scheme.
2.) expand each command definition with its own completer. So e.g. for start commands we could only list inactive domains, networks, pools, whatever. For destroy only active objects. And so on.
So maybe we want both - an option-based completer that generates the initial list, and a command-based completer that can then prune out irrelevant options? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 27.06.2011 21:39, Eric Blake wrote:
On 06/27/2011 12:06 PM, Michal Privoznik wrote:
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.
I was thinking about same feature and started to work on it during this weekend. But I ran into a problem. Basically, what I intended to implement, is:
1.) expand struct vshCmdDef for a char *(*callback) (const char *text, int state). But for real benefit, we need connection object, so we could e.g. list only inactive networks for 'net-start' command. And this is the problem, because we would have to make this object global (since readline functions does not allow passing any opaque value).
As gross as it is, a global object isn't entirely bad - virsh is single-threaded. And even if we want to make virsh threaded at some point, I still think we can get away with adding a thread-local access scheme.
2.) expand each command definition with its own completer. So e.g. for start commands we could only list inactive domains, networks, pools, whatever. For destroy only active objects. And so on.
So maybe we want both - an option-based completer that generates the initial list, and a command-based completer that can then prune out irrelevant options?
Well, I would say that is the highest goal to be achieved. Although that implies parsing during options generation. But I don't think I get it. Option-based completer generates list for given option, command-based completer generates list for given command (entries from this list are options). Take detach-interface for instance. This have some options: --mac: here we want option-based completer to list mac addresses of a NICs of a domain specified by --domain: since this could be left out we want here command-base completer to either list all domains, or list only those domains which have a NIC. So then we get: # detach-interface <TAB><TAB> --mac --persistent domain1 domain2 # detach-interface domain1 --mac <TAB><TAB> 00:11:22:33:44:55 01:23:45:67:89 My point is - I can't see any pruning here. But maybe I've chosen wrong example. However, this example shows we need both types of completer. Michal
On 06/28/2011 05:29 PM, Michal Privoznik wrote:
On 27.06.2011 21:39, Eric Blake wrote:
On 06/27/2011 12:06 PM, Michal Privoznik wrote:
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.
I was thinking about same feature and started to work on it during this weekend. But I ran into a problem. Basically, what I intended to implement, is:
1.) expand struct vshCmdDef for a char *(*callback) (const char *text, int state). But for real benefit, we need connection object, so we could e.g. list only inactive networks for 'net-start' command. And this is the problem, because we would have to make this object global (since readline functions does not allow passing any opaque value).
As gross as it is, a global object isn't entirely bad - virsh is single-threaded. And even if we want to make virsh threaded at some point, I still think we can get away with adding a thread-local access scheme.
2.) expand each command definition with its own completer. So e.g. for start commands we could only list inactive domains, networks, pools, whatever. For destroy only active objects. And so on.
So maybe we want both - an option-based completer that generates the initial list, and a command-based completer that can then prune out irrelevant options?
Well, I would say that is the highest goal to be achieved. Although that implies parsing during options generation.
But I don't think I get it. Option-based completer generates list for given option, command-based completer generates list for given command (entries from this list are options). Take detach-interface for instance. This have some options: --mac: here we want option-based completer to list mac addresses of a NICs of a domain specified by --domain: since this could be left out we want here command-base completer to either list all domains, or list only those domains which have a NIC.
So then we get:
# detach-interface <TAB><TAB> --mac --persistent domain1 domain2
# detach-interface domain1 --mac <TAB><TAB> 00:11:22:33:44:55 01:23:45:67:89
My point is - I can't see any pruning here. But maybe I've chosen wrong example. However, this example shows we need both types of completer.
Michal
Hi, All Are their any progress now? Thanks, Lai
On 20.07.2011 10:42, Lai Jiangshan wrote:
On 06/28/2011 05:29 PM, Michal Privoznik wrote:
On 27.06.2011 21:39, Eric Blake wrote:
On 06/27/2011 12:06 PM, Michal Privoznik wrote:
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.
I was thinking about same feature and started to work on it during this weekend. But I ran into a problem. Basically, what I intended to implement, is:
1.) expand struct vshCmdDef for a char *(*callback) (const char *text, int state). But for real benefit, we need connection object, so we could e.g. list only inactive networks for 'net-start' command. And this is the problem, because we would have to make this object global (since readline functions does not allow passing any opaque value).
As gross as it is, a global object isn't entirely bad - virsh is single-threaded. And even if we want to make virsh threaded at some point, I still think we can get away with adding a thread-local access scheme.
2.) expand each command definition with its own completer. So e.g. for start commands we could only list inactive domains, networks, pools, whatever. For destroy only active objects. And so on.
So maybe we want both - an option-based completer that generates the initial list, and a command-based completer that can then prune out irrelevant options?
Well, I would say that is the highest goal to be achieved. Although that implies parsing during options generation.
But I don't think I get it. Option-based completer generates list for given option, command-based completer generates list for given command (entries from this list are options). Take detach-interface for instance. This have some options: --mac: here we want option-based completer to list mac addresses of a NICs of a domain specified by --domain: since this could be left out we want here command-base completer to either list all domains, or list only those domains which have a NIC.
So then we get:
# detach-interface<TAB><TAB> --mac --persistent domain1 domain2
# detach-interface domain1 --mac<TAB><TAB> 00:11:22:33:44:55 01:23:45:67:89
My point is - I can't see any pruning here. But maybe I've chosen wrong example. However, this example shows we need both types of completer.
Michal
Hi, All
Are their any progress now?
Thanks, Lai
Unfortunately no. I've got distracted by other stuff (like QoS). But I agree it is feature which will increase user friendliness of virsh. No doubt about it. That's why I started to work on it. I hate typing whole names, if it is obvious which one I mean. Michal
participants (3)
-
Eric Blake -
Lai Jiangshan -
Michal Privoznik