Update _virsh completion

Update _virsh completion

[Marko Myllynen]

Hi,

The patch below (almost) completes _virsh completions:

* fix/improve help command completion
* fix/improve enabling file completion
* selective completion support for
  + domains
  + interfaces
  + networks
  + devices
  + nwfilters
  + pools
  + secrets
  + snapshots
  + volumes
* pass -c/--connect parameter as needed
  + thanks to Daniel for the suggestion
* sanitize parameters passed to commands run with sudo(8)
* cosmetics

The list of commands for which selective completion support has been
defined may not be complete (I didn't go through all the ~220 virsh
commands) but if there is no definition for a command then, e.g., all
domains instead of inactive domains are being offered.

Few remarks:

1) I wonder is there more elegant way to do this:

[[ ${(k)words[(I)--pool]} -gt 0 ]] && pool=${words[1+${(k)words[(I)--pool]}]}

2) For some reason virsh command options are offered more than once
even with this (-w):

+    [[ -n ${=_cache_virsh_cmd_opts[$cmd]} ]] && \
+      _values -w options ${=_cache_virsh_cmd_opts[$cmd]} && ret=0

For example:

$ virsh start --domain foo <TAB>
--autodestroy   --console       --pass-fds                                  
--bypass-cache  --force-boot    --paused                                    
$ virsh start --domain foo --autodestroy <TAB>
--autodestroy   --console       --pass-fds                                  
--bypass-cache  --force-boot    --paused                                    
$ virsh start --domain foo --autodestroy --autodestroy

3) The proper handling of calling sudo from completion function
was left open last time, the last email was:

http://www.zsh.org/mla/workers/2016/msg01406.html

It's unclear at least to me what would be the preferred
approach so I think a follow-up patch is needed if a consensus
emerges.


---
 Completion/Unix/Command/_libvirt | 153 +++++++++++++++++++++++++++++++--------
 1 file changed, 124 insertions(+), 29 deletions(-)

diff --git a/Completion/Unix/Command/_libvirt b/Completion/Unix/Command/_libvirt
index c855ac9..4fb9630 100644
--- a/Completion/Unix/Command/_libvirt
+++ b/Completion/Unix/Command/_libvirt
@@ -1,6 +1,7 @@
 #compdef virsh virt-admin virt-host-validate virt-pki-validate virt-xml-validate
 
 local curcontext="$curcontext" state line expl ret=1
+declare -A opt_args
 
 local exargs="-h --help -V -v --version=short --version=long"
 local -a common_opts interact_cmds
@@ -15,17 +16,48 @@ common_opts=(
 )
 interact_cmds=(cd echo exit quit connect)
 
+typeset -A dom_opts
+dom_opts=(
+  console " "
+  destroy " "
+  managedsave " "
+  reboot " "
+  reset " "
+  resume --state-paused
+  save " "
+  screenshot " "
+  send-key " "
+  shutdown --state-running
+  start --state-shutoff
+  suspend --state-running
+  ttyconsole " "
+  undefine --inactive
+  vncdisplay " "
+)
+typeset -A iface_opts
+iface_opts=(
+  iface-start --inactive
+)
+typeset -A net_opts
+net_opts=(
+  net-start --inactive
+)
+typeset -A pool_opts
+pool_opts=(
+  pool-start --inactive
+)
+
 case $service in
   virsh)
     if (( ! $+_cache_virsh_cmds )); then
-      _cache_virsh_cmds=( ${${${${(f):-"$(_call_program options virsh help)"}:#*:}/# ##}/ *} )
+      _cache_virsh_cmds=( ${${${${(f):-"$(_call_program commands virsh help)"}:#*:}/# ##}/ *} )
       local icmd
       for icmd in $interact_cmds; do
         _cache_virsh_cmds[$_cache_virsh_cmds[(i)$icmd]]=()
       done
     fi
-    if (( ! $+_cache_virsh_cmdopts )); then
-      typeset -gA _cache_virsh_cmdopts
+    if (( ! $+_cache_virsh_cmd_opts )); then
+      typeset -gA _cache_virsh_cmd_opts
     fi
     _arguments -A "-*" -C -S -s -w \
       "$common_opts[@]" \
@@ -35,28 +67,28 @@ case $service in
       "(-r --readonly $exargs)"{-r,--readonly}'[connect readonly]' \
       "(-t --timing $exargs)"{-t,--timing}'[print timing information]' \
       '1:command:->virsh_cmds' \
-      '*:cmdopt:->virsh_cmdopts' && return
+      '*:cmdopt:->virsh_cmd_opts' && return
       # We accept only virsh command options after the first non-option argument
       # (i.e., the virsh command itself), this makes it so with the -A "-*" above
-      [[ -z $state ]] && state=virsh_cmdopts
+      [[ -z $state ]] && state=virsh_cmd_opts
   ;;
   virt-admin)
     if (( ! $+_cache_virt_admin_cmds )); then
-      _cache_virt_admin_cmds=( ${${${${(f):-"$(_call_program options virt-admin help)"}:#*:}/# ##}/ *} )
+      _cache_virt_admin_cmds=( ${${${${(f):-"$(_call_program commands virt-admin help)"}:#*:}/# ##}/ *} )
       local icmd
       for icmd in $interact_cmds; do
         _cache_virt_admin_cmds[$_cache_virt_admin_cmds[(i)$icmd]]=()
       done
     fi
-    if (( ! $+_cache_virt_admin_cmdopts )); then
-      typeset -gA _cache_virt_admin_cmdopts
+    if (( ! $+_cache_virt_admin_cmd_opts )); then
+      typeset -gA _cache_virt_admin_cmd_opts
     fi
     _arguments -A "-*" -C -S -s -w \
       "$common_opts[@]" \
       '1:command:->virt_admin_cmds' \
-      '*:cmdopt:->virt_admin_cmdopts' && return
+      '*:cmdopt:->virt_admin_cmd_opts' && return
       # Same as with virsh above
-      [[ -z $state ]] && state=virt_admin_cmdopts
+      [[ -z $state ]] && state=virt_admin_cmd_opts
   ;;
   virt-host-validate)
     _arguments -A "-*" -S \
@@ -81,50 +113,113 @@ case $service in
   ;;
 esac
 
+local -a conn_opt
+if [[ -n ${(v)opt_args[(I)-c|--connect]} ]]; then
+  local uri=${(v)opt_args[(I)-c|--connect]}
+  [[ -z ${(Q)uri//([[:alnum:]]|+|:|\/|@|-|\.|\?|=)} ]] && \
+    conn_opt=( -c $uri )
+fi
+
 case $state in
   virsh_cmds)
     _wanted commands expl 'virsh command' compadd -a _cache_virsh_cmds && ret=0
   ;;
-  virsh_cmdopts)
-    local cmd
-    if [[ $words[-1] == /* || $words[-1] == ./* ]]; then
+  virsh_cmd_opts)
+    if [[ $words[-2] == --(dir|emulatorbin|file|mountpoint|*path|script|source-dev) || $words[-1] == (/*|.*) ]]; then
       _default
-      return
+      return 0
     fi
+    local cmd
     for (( i = 2; i <= $#words; i++ )); do
       [[ -n "${_cache_virsh_cmds[(r)$words[$i]]}" ]] && cmd=$words[$i] && break
     done
     [[ -z $cmd ]] && return 1
-    if [[ -z $_cache_virsh_cmdopts[$cmd] ]]; then
-      _cache_virsh_cmdopts[$cmd]=${(M)${${${${=${(f)"$(_call_program virsh virsh help $cmd 2>&1)"}}/\[}/\]}/\;}:#-[-0-9A-Za-z]*}
+    local -a values
+    case $words[-2] in
+      --domain)
+        values=( $(_call_program domains "virsh ${(Q)conn_opt} list ${dom_opts[$cmd]:-"--all"} --name") )
+        [[ -n $values ]] && _values domain ${=values} && return 0
+        return 1
+      ;;
+      --interface)
+        values=( ${${${${(f):-"$(_call_program interfaces "virsh ${(Q)conn_opt} iface-list ${iface_opts[$cmd]:-"--all"}")"}/ Name*/}:#---*}/  */} )
+        [[ -n $values ]] && _values interface ${=values} && return 0
+        return 1
+      ;;
+      --network)
+        values=( $(_call_program networks "virsh ${(Q)conn_opt} net-list ${net_opts[$cmd]:-"--all"} --name") )
+        [[ -n $values ]] && _values network ${=values} && return 0
+        return 1
+      ;;
+      --device)
+        values; values=( $(_call_program nodedevs "virsh ${(Q)conn_opt} nodedev-list") )
+        [[ -n $values ]] && _values device ${=values} && return 0
+        return 1
+      ;;
+      --nwfilter)
+        values=( ${${${${(f):-"$(_call_program nwfilters "virsh ${(Q)conn_opt} nwfilter-list")"}/ UUID*/}:#---*}/  */} )
+        [[ -n $values ]] && _values nwfilter ${=values} && return 0
+        return 1
+      ;;
+      --pool)
+        values=( ${${${${(f):-"$(_call_program pools "virsh ${(Q)conn_opt} pool-list ${pool_opts[$cmd]:-"--all"}")"}/ Name*/}:#---*}/  */} )
+        [[ -n $values ]] && _values pool ${=values} && return 0
+        return 1
+      ;;
+      --secret)
+        values=( ${${${${(f):-"$(_call_program secrets "virsh ${(Q)conn_opt} secret-list")"}/ UUID*/}:#---*}/  */} )
+        [[ -n $values ]] && _values secret ${=values} && return 0
+        return 1
+      ;;
+      --snapshotname)
+        local dom ; [[ ${(k)words[(I)--domain]} -gt 0 ]] && dom=${words[1+${(k)words[(I)--domain]}]}
+        [[ -z $dom ]] && return 1
+        values=( ${${${${(f):-"$(_call_program snapshots "virsh ${(Q)conn_opt} snapshot-list --domain $dom 2>/dev/null")"}/ Name*/}:#---*}/  */} )
+        [[ -n $values ]] && _values snapshot ${=values} && return 0
+        return 1
+      ;;
+      --vol)
+        local pool ; [[ ${(k)words[(I)--pool]} -gt 0 ]] && pool=${words[1+${(k)words[(I)--pool]}]}
+        [[ -z $pool ]] && return 1
+        values=( ${${${${(f):-"$(_call_program volumes "virsh ${(Q)conn_opt} vol-list --pool $pool 2>/dev/null")"}/ Name*/}:#---*}/  */} )
+        [[ -n $values ]] && _values volume ${=values} && return 0
+        return 1
+      ;;
+    esac
+    if [[ $cmd == help ]]; then
+      [[ $words[-1] == -* ]] && _values -w -- --command && return 0
+      if [[ $words[-2] == help || $words[-2] == --command ]]; then
+        _values commands ${=_cache_virsh_cmds} && return 0
+      fi
+      return 1
     fi
-    _values -w options ${=_cache_virsh_cmdopts[$cmd]} && ret=0
+    [[ -z $_cache_virsh_cmd_opts[$cmd] ]] && \
+      _cache_virsh_cmd_opts[$cmd]=${(M)${${${${=${(f)"$(_call_program virsh virsh help $cmd 2>&1)"}}/\[}/\]}/\;}:#-[-0-9A-Za-z]*}
+    [[ -n ${=_cache_virsh_cmd_opts[$cmd]} ]] && \
+      _values -w options ${=_cache_virsh_cmd_opts[$cmd]} && ret=0
   ;;
   virt_admin_cmds)
     _wanted commands expl 'virt-admin command' compadd -a _cache_virt_admin_cmds && ret=0
   ;;
-  virt_admin_cmdopts)
+  virt_admin_cmd_opts)
     local cmd
     for (( i = 2; i <= $#words; i++ )); do
       [[ -n "${_cache_virt_admin_cmds[(r)$words[$i]]}" ]] && cmd=$words[$i] && break
     done
     [[ -z $cmd ]] && return 1
     if [[ $words[-2] == --server ]]; then
-      _values servers ${=${(S)${${(f)$(sudo virt-admin srv-list)}##*--- }//[0-9]* }} && return 0
+      _values servers ${=${(S)${${(f)$(sudo virt-admin ${(Q)conn_opt} srv-list)}##*--- }//[0-9]* }} && return 0
     fi
     if [[ $words[-2] == --client ]]; then
-      local srv
-      for (( i = 2; i <= $#words; i++ )); do
-        [[ $words[$i] == --server ]] && srv=$words[$i+1] && break
-      done
+      local srv ; [[ ${(k)words[(I)--server]} -gt 0 ]] && srv=${words[1+${(k)words[(I)--server]}]}
       [[ -z $srv ]] && return 1
-      _values servers ${=${${(f):-"$(sudo virt-admin srv-clients-list --server $srv)"}/ [a-z]*}//[^0-9]} && return 0
-    fi
-    if [[ -z $_cache_virt_admin_cmdopts[$cmd] ]]; then
-      _cache_virt_admin_cmdopts[$cmd]=${(M)${${${${=${(f)"$(_call_program virt-admin virt-admin help $cmd 2>&1)"}}/\[}/\]}/\;}:#-[-0-9A-Za-z]*}
+      [[ -n ${srv//[[:alnum:]]} ]] && return 1
+      _values servers ${=${${(f):-"$(sudo virt-admin ${(Q)conn_opt} srv-clients-list --server $srv 2>/dev/null)"}/ [a-z]*}//[^0-9]} && return 0
     fi
-    [[ -n $_cache_virt_admin_cmdopts[$cmd] ]] && \
-      _values -w options ${=_cache_virt_admin_cmdopts[$cmd]} && ret=0
+    [[ -z $_cache_virt_admin_cmd_opts[$cmd] ]] && \
+      _cache_virt_admin_cmd_opts[$cmd]=${(M)${${${${=${(f)"$(_call_program virt-admin virt-admin help $cmd 2>&1)"}}/\[}/\]}/\;}:#-[-0-9A-Za-z]*}
+    [[ -n $_cache_virt_admin_cmd_opts[$cmd] ]] && \
+      _values -w options ${=_cache_virt_admin_cmd_opts[$cmd]} && ret=0
   ;;
 
 esac

Thanks,

-- 
Marko Myllynen

Re: Update _virsh completion

[Daniel Shahaf]

Marko Myllynen wrote on Mon, Aug 29, 2016 at 11:03:13 +0300:
> Hi,
> 
> The patch below (almost) completes _virsh completions:
> 
> * fix/improve help command completion
> * fix/improve enabling file completion
> * selective completion support for
>   + domains
>   + interfaces
>   + networks
>   + devices
>   + nwfilters
>   + pools
>   + secrets
>   + snapshots
>   + volumes
> * pass -c/--connect parameter as needed
>   + thanks to Daniel for the suggestion
> * sanitize parameters passed to commands run with sudo(8)
> * cosmetics
> 

Nice!

> The list of commands for which selective completion support has been
> defined may not be complete (I didn't go through all the ~220 virsh
> commands) but if there is no definition for a command then, e.g., all
> domains instead of inactive domains are being offered.
> 

Makes sense.

> Few remarks:
> 
> 1) I wonder is there more elegant way to do this:
> 
> [[ ${(k)words[(I)--pool]} -gt 0 ]] && pool=${words[1+${(k)words[(I)--pool]}]}
> 

Normally this is handled by $opt_args which is populated by _arguments.
Have you considered calling _arguments from the 'virsh subcommand
--<TAB>' completion?

This would also handle the --pool=foo (as a single argument word) syntax.

Secondarily, this code isn't idiomatic: the 'k' flag applies to
associative arrays, and (( )) is more idiomatic than [[ ]] for dealing
with integer arithmetics (compare users/21820).

> 2) For some reason virsh command options are offered more than once
> even with this (-w):
> 
> +    [[ -n ${=_cache_virsh_cmd_opts[$cmd]} ]] && \
> +      _values -w options ${=_cache_virsh_cmd_opts[$cmd]} && ret=0
> 
> $ virsh start --domain foo --autodestroy <TAB>
> --autodestroy   --console       --pass-fds                                  

That's because the expansion contains "--autodestroy" twice.  You can
easily uniquify the expansion with ${(u)foo} (or 'typeset -aU').

Maybe _values should do that automatically?

> 3) The proper handling of calling sudo from completion function
> was left open last time, the last email was:
> 
> http://www.zsh.org/mla/workers/2016/msg01406.html
> 
> It's unclear at least to me what would be the preferred
> approach so I think a follow-up patch is needed if a consensus
> emerges.

*nod*

workers@: Could people chime in please about the sudo question?

> -    if (( ! $+_cache_virsh_cmdopts )); then
> -      typeset -gA _cache_virsh_cmdopts
> +    if (( ! $+_cache_virsh_cmd_opts )); then
> +      typeset -gA _cache_virsh_cmd_opts

There's a cache mechanism in compsys, see the 'use-cache' style.
However, I don't know whether it would make sense to use it here.

(Not a blocker; can be done in a followup if needed)

>  esac
>  
> +local -a conn_opt
> +if [[ -n ${(v)opt_args[(I)-c|--connect]} ]]; then
> +  local uri=${(v)opt_args[(I)-c|--connect]}
> +  [[ -z ${(Q)uri//([[:alnum:]]|+|:|\/|@|-|\.|\?|=)} ]] && \
> +    conn_opt=( -c $uri )

A comment explaining the condition, please?

If you're trying to check whether $uri is shell-safe, see below about
using ${(q)conn_opt}.

> +  virsh_cmd_opts)
> +    ⋮
> +    case $words[-2] in

I think you want $words[CURRENT-1]; this matters when $SUFFIX is not
empty, i.e., when you complete at —

     % virsh subcommand --<CURSOR> --foo

> +      --vol)
> +        local pool ; [[ ${(k)words[(I)--pool]} -gt 0 ]] && pool=${words[1+${(k)words[(I)--pool]}]}
> +        [[ -z $pool ]] && return 1
> +        values=( ${${${${(f):-"$(_call_program volumes "virsh ${(Q)conn_opt} vol-list --pool $pool 2>/dev/null")"}/ Name*/}:#---*}/  */} )

That needs to be ${(q)pool}.  (I'm not sure whether pool names can
contain shell-unsafe characters, but I tend to always use ${(q)} since
it's never wrong.)  There are a few other instances of this, notably one
in a _call_program invocation that uses sudo.

Same question about the (Q): is it correct?  It would be correct if the
value of $conn_opt is twice shell-escaped — as though by ${(q)${(q)foo}}) —
which isn't the case here.

> +        [[ -n $values ]] && _values volume ${=values} && return 0
> +        return 1
> +      ;;
> +    esac
> +    if [[ $cmd == help ]]; then
> +      [[ $words[-1] == -* ]] && _values -w -- --command && return 0

«_values -w -- --command» doesn't do anything useful:

$ zsh -f
% autoload compinit && compinit
% _f() _values -w -- --foo
% compdef _f f
% f 
(eval):1: bad substitution
(eval):1: not an identifier: [_-]=* r:|=*[@]

Perhaps you meant «_values -w description -- --foo» or «_values -w
description --foo».  (The former actually adds the «--» as one of the
completions.) 

>

Cheers,

Daniel

Re: Update _virsh completion

[Oliver Kiddle]

Daniel Shahaf wrote:
> Marko Myllynen wrote on Mon, Aug 29, 2016 at 11:03:13 +0300:
> > The patch below (almost) completes _virsh completions:

Thanks. I committed it. Daniel has covered most of the points I wanted to
make.

One thing I found relating to virsh, having checked a system that
actually uses libvirt is that there was some virtual machines that
were listed with --inactive but not with --state-shutoff. virsh start
was able to start them.

> > $ virsh start --domain foo --autodestroy <TAB>
> > --autodestroy   --console       --pass-fds                                  
>
> That's because the expansion contains "--autodestroy" twice.  You can
> easily uniquify the expansion with ${(u)foo} (or 'typeset -aU').

I think it is caused by the presence of the word foo on the
line which is not one of the values passed to _values. I'd be inclined
to use _arguments instead of _values and have it handle things like
--domain taking and argument.

For a number of the other new uses of _values, _wanted would be
sufficient. It may be longer to write "_wanted tag expl desc compadd
…" than "_values desc …" but it does a lot less.  If you do stick
with _values, the convention for descriptions is not to use plural,
e.g server instead of servers. The description describes what should
be entered (e.g. a server) not what the listed matches are (e.g.
servers).

> > -    if (( ! $+_cache_virsh_cmdopts )); then
> > -      typeset -gA _cache_virsh_cmdopts
> > +    if (( ! $+_cache_virsh_cmd_opts )); then
> > +      typeset -gA _cache_virsh_cmd_opts
>
> There's a cache mechanism in compsys, see the 'use-cache' style.
> However, I don't know whether it would make sense to use it here.

That only really makes sense when generating the matches is particularly
slow which isn't the case here.

Oliver

Re: Update _virsh completion

[Daniel Shahaf]

Oliver Kiddle wrote on Wed, Aug 31, 2016 at 23:44:41 +0200:
> Daniel Shahaf wrote:
> > Marko Myllynen wrote on Mon, Aug 29, 2016 at 11:03:13 +0300:
> > > $ virsh start --domain foo --autodestroy <TAB>
> > > --autodestroy   --console       --pass-fds                                  
> >
> > That's because the expansion contains "--autodestroy" twice.  You can
> > easily uniquify the expansion with ${(u)foo} (or 'typeset -aU').
> 
> I think it is caused by the presence of the word foo on the
> line which is not one of the values passed to _values.

I can't reproduce the problem you're describing here.

I can reproduce the problem I described in my previous reply:

$ zsh -f
% autoload compinit; compinit
% _f() _values -w desc foo bar baz foo 
% compdef _f f
% f foo bar <TAB>
baz  foo

> The description describes what should be entered (e.g. a server) not
> what the listed matches are (e.g.  servers).

To clarify: the description tells the user how the shown values would be
used.  This is easiest to explain in terms of commands that takes
multiple arguments of the same type, like 'mv foo.txt bar.txt' might use
the descriptions "source file" and "target file".

(The example ignores some other syntaxes of mv(1) for simplicity.)

Cheers,

Daniel

Re: Update _virsh completion

[Marko Myllynen]

Hi,

Thanks for your review.

On 2016-08-31 20:49, Daniel Shahaf wrote:
> Marko Myllynen wrote on Mon, Aug 29, 2016 at 11:03:13 +0300:
> 
>> Few remarks:
>>
>> 1) I wonder is there more elegant way to do this:
>>
>> [[ ${(k)words[(I)--pool]} -gt 0 ]] && pool=${words[1+${(k)words[(I)--pool]}]}
> 
> Normally this is handled by $opt_args which is populated by _arguments.
> Have you considered calling _arguments from the 'virsh subcommand
> --<TAB>' completion?

No, I somehow thought _arguments is used with "main" commands and/or
states only, not in cases like this. I tried to look for examples but
couldn't find any so this is still unchanged, hope that's ok.

> Secondarily, this code isn't idiomatic: the 'k' flag applies to
> associative arrays, and (( )) is more idiomatic than [[ ]] for dealing
> with integer arithmetics (compare users/21820).

Ok, using (( ))) now instead.

>> 2) For some reason virsh command options are offered more than once
>> even with this (-w):
>>
>> +    [[ -n ${=_cache_virsh_cmd_opts[$cmd]} ]] && \
>> +      _values -w options ${=_cache_virsh_cmd_opts[$cmd]} && ret=0
>>
>> $ virsh start --domain foo --autodestroy <TAB>
>> --autodestroy   --console       --pass-fds                                  
> 
> That's because the expansion contains "--autodestroy" twice.  You can
> easily uniquify the expansion with ${(u)foo} (or 'typeset -aU').

Ah, of course, fixed.

> Maybe _values should do that automatically?

Not sure, perhaps best to leave it as is.

>> It's unclear at least to me what would be the preferred
>> approach so I think a follow-up patch is needed if a consensus
>> emerges.
> 
> *nod*
> 
> workers@: Could people chime in please about the sudo question?

This is now being discussed in the other thread.

>>  esac
>>  
>> +local -a conn_opt
>> +if [[ -n ${(v)opt_args[(I)-c|--connect]} ]]; then
>> +  local uri=${(v)opt_args[(I)-c|--connect]}
>> +  [[ -z ${(Q)uri//([[:alnum:]]|+|:|\/|@|-|\.|\?|=)} ]] && \
>> +    conn_opt=( -c $uri )
> 
> A comment explaining the condition, please?

Sure, it tries to make sure the URI is valid according to the spec,
added a link to the spec.

>> +  virsh_cmd_opts)
>> +    ⋮
>> +    case $words[-2] in
> 
> I think you want $words[CURRENT-1]; this matters when $SUFFIX is not
> empty, i.e., when you complete at —
> 
>      % virsh subcommand --<CURSOR> --foo

Yes, much better.

>> +      --vol)
>> +        local pool ; [[ ${(k)words[(I)--pool]} -gt 0 ]] && pool=${words[1+${(k)words[(I)--pool]}]}
>> +        [[ -z $pool ]] && return 1
>> +        values=( ${${${${(f):-"$(_call_program volumes "virsh ${(Q)conn_opt} vol-list --pool $pool 2>/dev/null")"}/ Name*/}:#---*}/  */} )
> 
> That needs to be ${(q)pool}.

Ok, fixed, works ok.

> Same question about the (Q): is it correct?  It would be correct if the
> value of $conn_opt is twice shell-escaped — as though by ${(q)${(q)foo}}) —
> which isn't the case here.

It was needed with sudo virt-admin commands but not with virsh
commands, it was there with virsh commands for consistency but indeed
makes sense to remove them there. With sudo commands without (Q) I see:

_values:compvalues:10: not enough arguments

When doing:

sudo virt-admin -c libvirtd:///system client-info --server libvirtd --client <TAB>

Looks like 'libvirtd\:///system' instead of 'libvirtd:///system' is
passed to virt-admin without (Q). Perhaps there's more optimal/safe
alternative here which I'm missing?

By the way, I noticed one corner case where the connection URI is not
passed properly, I think this is pretty uncommon but I wonder should
this be handled by _libvirt or perhaps in a more generic way?

$ unset LIBVIRT_DEFAULT_URI
$ virsh -c qemu:///system start --domain <TAB>
$ LIBVIRT_DEFAULT_URI=qemu:///system virsh start --domain <TAB>

So there the former works, the latter doesn't.

>> +        [[ -n $values ]] && _values volume ${=values} && return 0
>> +        return 1
>> +      ;;
>> +    esac
>> +    if [[ $cmd == help ]]; then
>> +      [[ $words[-1] == -* ]] && _values -w -- --command && return 0
> 
> «_values -w -- --command» doesn't do anything useful:
> 
> $ zsh -f
> % autoload compinit && compinit
> % _f() _values -w -- --foo
> % compdef _f f
> % f 
> (eval):1: bad substitution
> (eval):1: not an identifier: [_-]=* r:|=*[@]
> 
> Perhaps you meant «_values -w description -- --foo» or «_values -w
> description --foo».  (The former actually adds the «--» as one of the
> completions.) 

Yeah, braino, fixed.

I also adjusted the domains completed for virsh start and
_values/_wanted as suggested by Oliver, patch below.

---
 Completion/Unix/Command/_libvirt | 60 +++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/Completion/Unix/Command/_libvirt b/Completion/Unix/Command/_libvirt
index 4fb9630..ad4c3b8 100644
--- a/Completion/Unix/Command/_libvirt
+++ b/Completion/Unix/Command/_libvirt
@@ -28,7 +28,7 @@ dom_opts=(
   screenshot " "
   send-key " "
   shutdown --state-running
-  start --state-shutoff
+  start --inactive
   suspend --state-running
   ttyconsole " "
   undefine --inactive
@@ -116,6 +116,8 @@ esac
 local -a conn_opt
 if [[ -n ${(v)opt_args[(I)-c|--connect]} ]]; then
   local uri=${(v)opt_args[(I)-c|--connect]}
+  # For the libvirt remote URI syntax, see:
+  # https://libvirt.org/guide/html/Application_Development_Guide-Architecture-Remote_URIs.html
   [[ -z ${(Q)uri//([[:alnum:]]|+|:|\/|@|-|\.|\?|=)} ]] && \
     conn_opt=( -c $uri )
 fi
@@ -135,68 +137,68 @@ case $state in
     done
     [[ -z $cmd ]] && return 1
     local -a values
-    case $words[-2] in
+    case $words[CURRENT-1] in
       --domain)
-        values=( $(_call_program domains "virsh ${(Q)conn_opt} list ${dom_opts[$cmd]:-"--all"} --name") )
-        [[ -n $values ]] && _values domain ${=values} && return 0
+        values=( $(_call_program domains "virsh $conn_opt list ${dom_opts[$cmd]:-"--all"} --name") )
+        [[ -n $values ]] && _wanted domains expl domain compadd ${=values} && return 0
         return 1
       ;;
       --interface)
-        values=( ${${${${(f):-"$(_call_program interfaces "virsh ${(Q)conn_opt} iface-list ${iface_opts[$cmd]:-"--all"}")"}/ Name*/}:#---*}/  */} )
-        [[ -n $values ]] && _values interface ${=values} && return 0
+        values=( ${${${${(f):-"$(_call_program interfaces "virsh $conn_opt iface-list ${iface_opts[$cmd]:-"--all"}")"}/ Name*/}:#---*}/  */} )
+        [[ -n $values ]] && _wanted interfaces expl interface compadd ${=values} && return 0
         return 1
       ;;
       --network)
-        values=( $(_call_program networks "virsh ${(Q)conn_opt} net-list ${net_opts[$cmd]:-"--all"} --name") )
-        [[ -n $values ]] && _values network ${=values} && return 0
+        values=( $(_call_program networks "virsh $conn_opt net-list ${net_opts[$cmd]:-"--all"} --name") )
+        [[ -n $values ]] && _wanted networks expl network compadd ${=values} && return 0
         return 1
       ;;
       --device)
-        values; values=( $(_call_program nodedevs "virsh ${(Q)conn_opt} nodedev-list") )
-        [[ -n $values ]] && _values device ${=values} && return 0
+        values; values=( $(_call_program nodedevs "virsh $conn_opt nodedev-list") )
+        [[ -n $values ]] && _wanted devices expl device compadd ${=values} && return 0
         return 1
       ;;
       --nwfilter)
-        values=( ${${${${(f):-"$(_call_program nwfilters "virsh ${(Q)conn_opt} nwfilter-list")"}/ UUID*/}:#---*}/  */} )
-        [[ -n $values ]] && _values nwfilter ${=values} && return 0
+        values=( ${${${${(f):-"$(_call_program nwfilters "virsh $conn_opt nwfilter-list")"}/ UUID*/}:#---*}/  */} )
+        [[ -n $values ]] && _wanted nwfilters expl nwfilter compadd ${=values} && return 0
         return 1
       ;;
       --pool)
-        values=( ${${${${(f):-"$(_call_program pools "virsh ${(Q)conn_opt} pool-list ${pool_opts[$cmd]:-"--all"}")"}/ Name*/}:#---*}/  */} )
-        [[ -n $values ]] && _values pool ${=values} && return 0
+        values=( ${${${${(f):-"$(_call_program pools "virsh $conn_opt pool-list ${pool_opts[$cmd]:-"--all"}")"}/ Name*/}:#---*}/  */} )
+        [[ -n $values ]] && _wanted pools expl pool compadd ${=values} && return 0
         return 1
       ;;
       --secret)
-        values=( ${${${${(f):-"$(_call_program secrets "virsh ${(Q)conn_opt} secret-list")"}/ UUID*/}:#---*}/  */} )
-        [[ -n $values ]] && _values secret ${=values} && return 0
+        values=( ${${${${(f):-"$(_call_program secrets "virsh $conn_opt secret-list")"}/ UUID*/}:#---*}/  */} )
+        [[ -n $values ]] && _wanted secrets expl secret compadd ${=values} && return 0
         return 1
       ;;
       --snapshotname)
-        local dom ; [[ ${(k)words[(I)--domain]} -gt 0 ]] && dom=${words[1+${(k)words[(I)--domain]}]}
+        local dom ; (( ${(k)words[(I)--domain]} > 0 )) && dom=${words[1+${(k)words[(I)--domain]}]}
         [[ -z $dom ]] && return 1
-        values=( ${${${${(f):-"$(_call_program snapshots "virsh ${(Q)conn_opt} snapshot-list --domain $dom 2>/dev/null")"}/ Name*/}:#---*}/  */} )
-        [[ -n $values ]] && _values snapshot ${=values} && return 0
+        values=( ${${${${(f):-"$(_call_program snapshots "virsh $conn_opt snapshot-list --domain ${(q)dom} 2>/dev/null")"}/ Name*/}:#---*}/  */} )
+        [[ -n $values ]] && _wanted snapshots expl snapshot compadd ${=values} && return 0
         return 1
       ;;
       --vol)
-        local pool ; [[ ${(k)words[(I)--pool]} -gt 0 ]] && pool=${words[1+${(k)words[(I)--pool]}]}
+        local pool ; (( ${(k)words[(I)--pool]} > 0 )) && pool=${words[1+${(k)words[(I)--pool]}]}
         [[ -z $pool ]] && return 1
-        values=( ${${${${(f):-"$(_call_program volumes "virsh ${(Q)conn_opt} vol-list --pool $pool 2>/dev/null")"}/ Name*/}:#---*}/  */} )
-        [[ -n $values ]] && _values volume ${=values} && return 0
+        values=( ${${${${(f):-"$(_call_program volumes "virsh $conn_opt vol-list --pool ${(q)pool} 2>/dev/null")"}/ Name*/}:#---*}/  */} )
+        [[ -n $values ]] && _wanted volumes expl volume compadd ${=values} && return 0
         return 1
       ;;
     esac
     if [[ $cmd == help ]]; then
-      [[ $words[-1] == -* ]] && _values -w -- --command && return 0
+      [[ $words[-1] == -* ]] && _values -w option --command && return 0
       if [[ $words[-2] == help || $words[-2] == --command ]]; then
-        _values commands ${=_cache_virsh_cmds} && return 0
+        _wanted commands expl command compadd ${=_cache_virsh_cmds} && return 0
       fi
       return 1
     fi
     [[ -z $_cache_virsh_cmd_opts[$cmd] ]] && \
       _cache_virsh_cmd_opts[$cmd]=${(M)${${${${=${(f)"$(_call_program virsh virsh help $cmd 2>&1)"}}/\[}/\]}/\;}:#-[-0-9A-Za-z]*}
     [[ -n ${=_cache_virsh_cmd_opts[$cmd]} ]] && \
-      _values -w options ${=_cache_virsh_cmd_opts[$cmd]} && ret=0
+      _values -w option ${(u)=_cache_virsh_cmd_opts[$cmd]} && ret=0
   ;;
   virt_admin_cmds)
     _wanted commands expl 'virt-admin command' compadd -a _cache_virt_admin_cmds && ret=0
@@ -208,18 +210,18 @@ case $state in
     done
     [[ -z $cmd ]] && return 1
     if [[ $words[-2] == --server ]]; then
-      _values servers ${=${(S)${${(f)$(sudo virt-admin ${(Q)conn_opt} srv-list)}##*--- }//[0-9]* }} && return 0
+      _wanted servers expl server compadd ${=${(S)${${(f)$(sudo virt-admin ${(Q)conn_opt} srv-list)}##*--- }//[0-9]* }} && return 0
     fi
     if [[ $words[-2] == --client ]]; then
-      local srv ; [[ ${(k)words[(I)--server]} -gt 0 ]] && srv=${words[1+${(k)words[(I)--server]}]}
+      local srv ; (( ${(k)words[(I)--server]} > 0 )) && srv=${words[1+${(k)words[(I)--server]}]}
       [[ -z $srv ]] && return 1
       [[ -n ${srv//[[:alnum:]]} ]] && return 1
-      _values servers ${=${${(f):-"$(sudo virt-admin ${(Q)conn_opt} srv-clients-list --server $srv 2>/dev/null)"}/ [a-z]*}//[^0-9]} && return 0
+      _wanted clients expl client compadd ${=${${(f):-"$(sudo virt-admin ${(Q)conn_opt} srv-clients-list --server $srv 2>/dev/null)"}/ [a-z]*}//[^0-9]} && return 0
     fi
     [[ -z $_cache_virt_admin_cmd_opts[$cmd] ]] && \
       _cache_virt_admin_cmd_opts[$cmd]=${(M)${${${${=${(f)"$(_call_program virt-admin virt-admin help $cmd 2>&1)"}}/\[}/\]}/\;}:#-[-0-9A-Za-z]*}
     [[ -n $_cache_virt_admin_cmd_opts[$cmd] ]] && \
-      _values -w options ${=_cache_virt_admin_cmd_opts[$cmd]} && ret=0
+      _values -w option ${(u)=_cache_virt_admin_cmd_opts[$cmd]} && ret=0
   ;;
 
 esac

Thanks,

-- 
Marko Myllynen

Re: Update _virsh completion

[Daniel Shahaf]

Marko Myllynen wrote on Fri, Sep 02, 2016 at 12:36:44 +0300:
> Hi,
> 
> Thanks for your review.
> 
> On 2016-08-31 20:49, Daniel Shahaf wrote:
> > Marko Myllynen wrote on Mon, Aug 29, 2016 at 11:03:13 +0300:
> > 
> >> Few remarks:
> >>
> >> 1) I wonder is there more elegant way to do this:
> >>
> >> [[ ${(k)words[(I)--pool]} -gt 0 ]] && pool=${words[1+${(k)words[(I)--pool]}]}
> > 
> > Normally this is handled by $opt_args which is populated by _arguments.
> > Have you considered calling _arguments from the 'virsh subcommand
> > --<TAB>' completion?
> 
> No, I somehow thought _arguments is used with "main" commands and/or
> states only, not in cases like this. I tried to look for examples but
> couldn't find any so this is still unchanged, hope that's ok.

I think _values is fine for now, but if you were to add support for the
--foo=bar syntax, or per-option description strings, the easiest way to
do so would be to switch from _values to _arguments.

There are a few examples of _arguments with subcommands: _git (lines
7446 and 7473 in _git revision 4547897976f0), _zpool, _rpm.  (The latter
is referred from the last 15 lines of Etc/completion-style-guide.)

> >> +      --vol)
> >> +        local pool ; [[ ${(k)words[(I)--pool]} -gt 0 ]] && pool=${words[1+${(k)words[(I)--pool]}]}
> >> +        [[ -z $pool ]] && return 1
> >> +        values=( ${${${${(f):-"$(_call_program volumes "virsh ${(Q)conn_opt} vol-list --pool $pool 2>/dev/null")"}/ Name*/}:#---*}/  */} )
> > 
> > That needs to be ${(q)pool}.
> 
> Ok, fixed, works ok.
> 

Sorry, I may have mislead you here.  I thought the rule was "all
parameter expansions should be (q)'d", but I hadn't realised then that
elements of $words were already quoted once.  I'm not sure now what's
the correct way to insert an element of $words into the argument to
_call_program.

Off the top of my head, this is used by _libvirt [virsh -c], _postfix
[postconf -c], and  as _git-config.

(see more below in the discussion about LIBVIRT_DEFAULT_URI)

> > Same question about the (Q): is it correct?  It would be correct if the
> > value of $conn_opt is twice shell-escaped — as though by ${(q)${(q)foo}}) —
> > which isn't the case here.
> 
> It was needed with sudo virt-admin commands but not with virsh
> commands, it was there with virsh commands for consistency but indeed
> makes sense to remove them there. With sudo commands without (Q) I see:
> 
> _values:compvalues:10: not enough arguments
> 
> When doing:
> 
> sudo virt-admin -c libvirtd:///system client-info --server libvirtd --client <TAB>
> 
> Looks like 'libvirtd\:///system' instead of 'libvirtd:///system' is
> passed to virt-admin without (Q). Perhaps there's more optimal/safe
> alternative here which I'm missing?
> 

I'm not sure why the colon gets escaped.  Typically, colons get escaped
when constructing a spec argument to _arguments or _describe.

> By the way, I noticed one corner case where the connection URI is not
> passed properly, I think this is pretty uncommon but I wonder should
> this be handled by _libvirt or perhaps in a more generic way?
> 
> $ unset LIBVIRT_DEFAULT_URI
> $ virsh -c qemu:///system start --domain <TAB>
> $ LIBVIRT_DEFAULT_URI=qemu:///system virsh start --domain <TAB>
> 
> So there the former works, the latter doesn't.

How would you handle

    % LIBVIRT_DEFAULT_URI=foo://bar
    % unset LIBVIRT_DEFAULT_URI; virsh start <TAB>

or

    % unset LIBVIRT_DEFAULT_URI
    % LIBVIRT_DEFAULT_URI=$(/this/command/is/not/idempotent/or/not/deterministic) virsh start <TAB>

?  Both in the «_call_program foo ...$words[N]...» case above and in the
two cases here, the question is how much it is okay to eval the argument
at a <TAB>.

(By the way, zsh-syntax-highlighting faces a similar issue, for example
when it needs to determine whether a command word, which is a parameter
expansion, refers to an alias or to a function.)

> I also adjusted the domains completed for virsh start and
> _values/_wanted as suggested by Oliver, patch below.

Thanks, looks good to me.  I assume Oliver will see it later.

Cheers,

Daniel

Re: Update _virsh completion

[Daniel Shahaf]

Daniel Shahaf wrote on Fri, Sep 02, 2016 at 18:20:24 +0000:
> Marko Myllynen wrote on Fri, Sep 02, 2016 at 12:36:44 +0300:
> > Looks like 'libvirtd\:///system' instead of 'libvirtd:///system' is
> > passed to virt-admin without (Q). Perhaps there's more optimal/safe
> > alternative here which I'm missing?
> > 
> 
> I'm not sure why the colon gets escaped.  Typically, colons get escaped
> when constructing a spec argument to _arguments or _describe.

The colon is escaped by _arguments:
.
                                        For options that have more than one
              argument these are given as one string, separated by colons.
              All colons in the original arguments are preceded with
              backslashes.
.
By "more than one argument" the manual refers to specs such as
«_arguments : -c:arg1:_users:arg2:_hosts».  This spec means -c takes
two arguments in separate words.   (Example: «foo -c danielsh localhost»
would yield «opt_args=(-c danielsh:localhost)».)

Undoing that escaping would be:

diff --git a/Completion/Unix/Command/_libvirt b/Completion/Unix/Command/_libvirt
index ad4c3b8..f887426 100644
--- a/Completion/Unix/Command/_libvirt
+++ b/Completion/Unix/Command/_libvirt
@@ -116,6 +116,7 @@ esac
 local -a conn_opt
 if [[ -n ${(v)opt_args[(I)-c|--connect]} ]]; then
   local uri=${(v)opt_args[(I)-c|--connect]}
+  uri=${uri//(#m)\\([\\:])/${MATCH[2]}} # opt_args elements are colon-escaped
   # For the libvirt remote URI syntax, see:
   # https://libvirt.org/guide/html/Application_Development_Guide-Architecture-Remote_URIs.html
   [[ -z ${(Q)uri//([[:alnum:]]|+|:|\/|@|-|\.|\?|=)} ]] && \

This patch changes the behaviour of «virsh -c qemu\:///system start
--domain <TAB>».

See also the _arguments email I'm about to send.

Cheers,

Daniel

Re: Update _virsh completion

[Marko Myllynen]

Hi,

On 2016-09-02 21:20, Daniel Shahaf wrote:
> Marko Myllynen wrote on Fri, Sep 02, 2016 at 12:36:44 +0300:
> 
>> By the way, I noticed one corner case where the connection URI is not
>> passed properly, I think this is pretty uncommon but I wonder should
>> this be handled by _libvirt or perhaps in a more generic way?
>>
>> $ unset LIBVIRT_DEFAULT_URI
>> $ virsh -c qemu:///system start --domain <TAB>
>> $ LIBVIRT_DEFAULT_URI=qemu:///system virsh start --domain <TAB>
>>
>> So there the former works, the latter doesn't.
> 
> How would you handle
> 
>     % LIBVIRT_DEFAULT_URI=foo://bar
>     % unset LIBVIRT_DEFAULT_URI; virsh start <TAB>
> 
> or
> 
>     % unset LIBVIRT_DEFAULT_URI
>     % LIBVIRT_DEFAULT_URI=$(/this/command/is/not/idempotent/or/not/deterministic) virsh start <TAB>
> 
> ?  Both in the «_call_program foo ...$words[N]...» case above and in the
> two cases here, the question is how much it is okay to eval the argument
> at a <TAB>.
> 
> (By the way, zsh-syntax-highlighting faces a similar issue, for example
> when it needs to determine whether a command word, which is a parameter
> expansion, refers to an alias or to a function.)

Good points, yeah, can't see an easy solution to those but since at
least in the case of _libvirt it's likely to be a corner case shouldn't
be too much of an issue.

Thanks,

-- 
Marko Myllynen