1:42 a.m.
Custom readline generator will help for some usecase.
Also add a custom readline generator for the "help" command.
Signed-off-by: Lai Jiangshan <laijs(a)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;
12:37 p.m.
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>
---
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:06 p.m.
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(a)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
2:39 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:29 a.m.
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
3:42 a.m.
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
3:50 a.m.
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
4874
days inactive
4900
days old
6 comments
3 participants
participants (3)
-
Eric Blake -
Lai Jiangshan -
Michal Privoznik