From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17756 invoked by alias); 2 Sep 2016 13:38:39 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 39158 Received: (qmail 3615 invoked from network); 2 Sep 2016 13:38:38 -0000 X-Qmail-Scanner-Diagnostics: from mail-wm0-f53.google.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.99.2/21882. spamassassin: 3.4.1. Clear:RC:0(74.125.82.53):SA:0(-0.0/5.0):. Processed in 0.64563 secs); 02 Sep 2016 13:38:38 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-0.0 required=5.0 tests=SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.1 X-Envelope-From: myllynen@redhat.com X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | Received-SPF: pass (ns1.primenet.com.au: SPF record at _netblocks.google.com designates 74.125.82.53 as permitted sender) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:reply-to:subject:references:to:cc:from :organization:message-id:date:user-agent:mime-version:in-reply-to :content-transfer-encoding; bh=G4Ee8gKsZu3MUanXZ7cULPx04cHYthy0i7wdzz3QQbc=; b=bMutBEpmOmbCFu3edJiusivPCpP+hA2g85RwsjwrsqqVdYP5KjrpTX73Pftkz6Cs0i SZf2Q4ZPGr5QA5o6+FBuj0TSFdxf2gPcPjvaov//sTcEmE6lxMENx1i0S1ISiEVbtKNy irOjis2uIq2RFZ2Z7qC2Nc3SCAf4eQJOoRvh/woxFEpDk/BGMNtGdlF0CXbfGou/LtMu cF7d+u+5BgfD761p1dKmGy93zBjPiS8Ke816jmk0zxmlDWBqCuQEeIIV73QIcX4UxAsn zekW6xRJmHni949O6nGOnnT2iHimxEkEK5VzQVWhPd0DkmOODWoNKGmaog1AaJ83Z2ek I71w== X-Gm-Message-State: AE9vXwMyeVaUIx+mnyMIqcc4lX6d95fs6Qai/m/4TmhGivym7UK1+/q9PulxIom+B0b1JD6o X-Received: by 10.28.128.15 with SMTP id b15mr2266520wmd.100.1472809008991; Fri, 02 Sep 2016 02:36:48 -0700 (PDT) Reply-To: Marko Myllynen Subject: Re: Update _virsh completion References: <20160831174915.GA11043@fujitsu.shahaf.local2> To: Daniel Shahaf Cc: zsh workers From: Marko Myllynen Organization: Red Hat Message-ID: Date: Fri, 2 Sep 2016 12:36:44 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2 MIME-Version: 1.0 In-Reply-To: <20160831174915.GA11043@fujitsu.shahaf.local2> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 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 > --' 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 >> --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 -- --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 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 $ LIBVIRT_DEFAULT_URI=qemu:///system virsh start --domain 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