Some fixes and improvement for virsh completion in non interactive mode

[libvirt] [PATCH v2 03/12] virsh: Conditionally Ignore the first entry in list of completions

Posted by Lin Ma 7 years, 9 months ago

The first entry in the returned array is the substitution for TEXT. It
causes unnecessary output if other commands or options share the same
prefix, e.g.

$ virsh des<TAB><TAB>
des      desc     destroy

or

$ virsh domblklist --d<TAB><TAB>
--d        --details  --domain

This patch fixes the above issue.

Signed-off-by: Lin Ma <lma@suse.com>
---
 tools/vsh.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index 73ec007e56..57f7589b53 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3458,6 +3458,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
     const vshCmdOpt *opt = NULL;
     char **matches = NULL, **iter;
     virBuffer buf = VIR_BUFFER_INITIALIZER;
+    int n;
 
     if (vshCommandOptStringQuiet(ctl, cmd, "string", &arg) <= 0)
         goto cleanup;
@@ -3493,8 +3494,11 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
     if (!(matches = vshReadlineCompletion(arg, 0, 0)))
         goto cleanup;
 
-    for (iter = matches; *iter; iter++)
+    for (n = 0, iter = matches; *iter; iter++, n++) {
+        if (n == 0 && matches[1])
+            continue;
         printf("%s\n", *iter);
+    }
 
     ret = true;
  cleanup:
-- 
2.15.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 03/12] virsh: Conditionally Ignore the first entry in list of completions

Posted by Michal Privoznik 7 years, 9 months ago

On 05/08/2018 04:20 PM, Lin Ma wrote:
> The first entry in the returned array is the substitution for TEXT. It
> causes unnecessary output if other commands or options share the same
> prefix, e.g.
> 
> $ virsh des<TAB><TAB>
> des      desc     destroy
> 
> or
> 
> $ virsh domblklist --d<TAB><TAB>
> --d        --details  --domain
> 
> This patch fixes the above issue.
> 
> Signed-off-by: Lin Ma <lma@suse.com>
> ---
>  tools/vsh.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/vsh.c b/tools/vsh.c
> index 73ec007e56..57f7589b53 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -3458,6 +3458,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
>      const vshCmdOpt *opt = NULL;
>      char **matches = NULL, **iter;
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    int n;

This needs to be size_t. Even though it's not used to directly access
entries of @matches array, it kind of is. It's used to count them. And
int is not guaranteed to be able to address all of them.

>  
>      if (vshCommandOptStringQuiet(ctl, cmd, "string", &arg) <= 0)
>          goto cleanup;
> @@ -3493,8 +3494,11 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
>      if (!(matches = vshReadlineCompletion(arg, 0, 0)))
>          goto cleanup;
>  
> -    for (iter = matches; *iter; iter++)
> +    for (n = 0, iter = matches; *iter; iter++, n++) {
> +        if (n == 0 && matches[1])
> +            continue;

This can be rewritten so that we don't need @n at all:

  if (iter == matches && matches[1])
      continue;

Or even better, just start iterating not from the first but second entry:

    for (iter = &matches[1]; *iter; iter++)
        printf("%s\n", *iter);

This is safe because:
a) @matches is guaranteed to be non-NULL at this point (due to check above),
b) @matches is NULL terminated array and as you and documentation [1]
say, the first entry is substitution for text (which we want to skip).

So even if the array is nothing but the substitution and NULL,
matches[1] will be NULL:

  matches = { "subs", NULL };

Also, I'm adding a small comment because it is not obvious why we are
skipping the first entry.

ACK then.

Michal

1: http://www.delorie.com/gnu/docs/readline/rlman_46.html

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 03/12] virsh: Conditionally Ignore the first entry in list of completions

Posted by Lin Ma 7 years, 9 months ago


On 05/10/2018 05:17 PM, Michal Privoznik wrote:
> On 05/08/2018 04:20 PM, Lin Ma wrote:
>> The first entry in the returned array is the substitution for TEXT. It
>> causes unnecessary output if other commands or options share the same
>> prefix, e.g.
>>
>> $ virsh des<TAB><TAB>
>> des      desc     destroy
>>
>> or
>>
>> $ virsh domblklist --d<TAB><TAB>
>> --d        --details  --domain
>>
>> This patch fixes the above issue.
>>
>> Signed-off-by: Lin Ma <lma@suse.com>
>> ---
>>   tools/vsh.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/vsh.c b/tools/vsh.c
>> index 73ec007e56..57f7589b53 100644
>> --- a/tools/vsh.c
>> +++ b/tools/vsh.c
>> @@ -3458,6 +3458,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
>>       const vshCmdOpt *opt = NULL;
>>       char **matches = NULL, **iter;
>>       virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +    int n;
> This needs to be size_t. Even though it's not used to directly access
> entries of @matches array, it kind of is. It's used to count them. And
> int is not guaranteed to be able to address all of them.
>
>>   
>>       if (vshCommandOptStringQuiet(ctl, cmd, "string", &arg) <= 0)
>>           goto cleanup;
>> @@ -3493,8 +3494,11 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
>>       if (!(matches = vshReadlineCompletion(arg, 0, 0)))
>>           goto cleanup;
>>   
>> -    for (iter = matches; *iter; iter++)
>> +    for (n = 0, iter = matches; *iter; iter++, n++) {
>> +        if (n == 0 && matches[1])
>> +            continue;
> This can be rewritten so that we don't need @n at all:
>
>    if (iter == matches && matches[1])
>        continue;
>
> Or even better, just start iterating not from the first but second entry:
>
>      for (iter = &matches[1]; *iter; iter++)
>          printf("%s\n", *iter);
It seems it can't handle the case that no command or option share the 
same prefix.
say 'event' command, when users type virsh ev<TAB><TAB>, there is no 
other command
sharing 'ev' prefix, in this case, the matches[1] is NULL and we need to 
print the
value in matches[0],I think we can't skip the first entry in this case.
So the code block 'if (iter == matches && matches[1])  continue;' looks 
like a better
choice.If you think so,
I'd like to write a patch to do it, Do you agree?
meanwhile, I want to remove those comment due to we can't skip first 
entry, Do you agree?

> This is safe because:
> a) @matches is guaranteed to be non-NULL at this point (due to check above),
> b) @matches is NULL terminated array and as you and documentation [1]
> say, the first entry is substitution for text (which we want to skip).
>
> So even if the array is nothing but the substitution and NULL,
> matches[1] will be NULL:
>
>    matches = { "subs", NULL };
>
> Also, I'm adding a small comment because it is not obvious why we are
> skipping the first entry.
>
> ACK then.
>
> Michal
>
> 1: http://www.delorie.com/gnu/docs/readline/rlman_46.html
>
Thanks,
Lin

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 03/12] virsh: Conditionally Ignore the first entry in list of completions

Posted by Michal Privoznik 7 years, 9 months ago

On 05/11/2018 10:01 AM, Lin Ma wrote:
> 
> 
> On 05/10/2018 05:17 PM, Michal Privoznik wrote:
>> On 05/08/2018 04:20 PM, Lin Ma wrote:
>>> The first entry in the returned array is the substitution for TEXT. It
>>> causes unnecessary output if other commands or options share the same
>>> prefix, e.g.
>>>
>>> $ virsh des<TAB><TAB>
>>> des      desc     destroy
>>>
>>> or
>>>
>>> $ virsh domblklist --d<TAB><TAB>
>>> --d        --details  --domain
>>>
>>> This patch fixes the above issue.
>>>
>>> Signed-off-by: Lin Ma <lma@suse.com>
>>> ---
>>>   tools/vsh.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/vsh.c b/tools/vsh.c
>>> index 73ec007e56..57f7589b53 100644
>>> --- a/tools/vsh.c
>>> +++ b/tools/vsh.c
>>> @@ -3458,6 +3458,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
>>>       const vshCmdOpt *opt = NULL;
>>>       char **matches = NULL, **iter;
>>>       virBuffer buf = VIR_BUFFER_INITIALIZER;
>>> +    int n;
>> This needs to be size_t. Even though it's not used to directly access
>> entries of @matches array, it kind of is. It's used to count them. And
>> int is not guaranteed to be able to address all of them.
>>
>>>         if (vshCommandOptStringQuiet(ctl, cmd, "string", &arg) <= 0)
>>>           goto cleanup;
>>> @@ -3493,8 +3494,11 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
>>>       if (!(matches = vshReadlineCompletion(arg, 0, 0)))
>>>           goto cleanup;
>>>   -    for (iter = matches; *iter; iter++)
>>> +    for (n = 0, iter = matches; *iter; iter++, n++) {
>>> +        if (n == 0 && matches[1])
>>> +            continue;
>> This can be rewritten so that we don't need @n at all:
>>
>>    if (iter == matches && matches[1])
>>        continue;
>>
>> Or even better, just start iterating not from the first but second entry:
>>
>>      for (iter = &matches[1]; *iter; iter++)
>>          printf("%s\n", *iter);
> It seems it can't handle the case that no command or option share the
> same prefix.
> say 'event' command, when users type virsh ev<TAB><TAB>, there is no
> other command
> sharing 'ev' prefix, in this case, the matches[1] is NULL and we need to
> print the
> value in matches[0],I think we can't skip the first entry in this case.
> So the code block 'if (iter == matches && matches[1])  continue;' looks
> like a better
> choice.If you think so,
> I'd like to write a patch to do it, Do you agree?
> meanwhile, I want to remove those comment due to we can't skip first
> entry, Do you agree?

Ah good catch. You're right, the documentation is not valid then. Sure,
if you can write the patch I'm happy to review it.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list