Re: Update _virsh completion

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

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