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@xxxxxxxxxxxxxx>
>> ---
>
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list