Re: Update _virsh completion

[Marko Myllynen <myllynen@xxxxxxxxxx>]

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