* _libvirt, $opt_args, and noglob @ 2016-09-04 18:26 Daniel Shahaf 2016-09-06 23:46 ` Daniel Shahaf 0 siblings, 1 reply; 4+ messages in thread From: Daniel Shahaf @ 2016-09-04 18:26 UTC (permalink / raw) To: zsh-workers; +Cc: Marko Myllynen The question mark in «noglob virsh -c foo://bar?» is interpreted by the _call_program subshell as a pattern, causing _libvirt to fail to find completions: (setup) % zstyle :completion:\*:warnings format 'No matches: %D' % functions -T $_comps[libvirt] % noglob virsh -c qemu:///system? start --domain <TAB> ⋮ +_libvirt:142> values=+_libvirt:142> _call_program domains 'virsh -c qemu\:///system? list --inactive --name' +_libvirt:142> values=( ) No matches: cmdopt Now create a filename that matches that globbing pattern and try the command again: % noglob mkdir -p ./qemu:/system? % noglob virsh -c qemu:///system? start --domain <TAB> <works> I believe the reason it works is that the globbing pattern «qemu:///system?» gets expanded into the filename «qemu:/system?», which happens to work just fine as an argument to «virsh -c». (The backslash in front of the colon here is an independent problem, see 39158.) In the special case of _libvirt, simply adding noglob to the _call_program arguments would probably fix this. This might not work for arbitrary other callsites, though. I think the general fix would be to backslash-escape the characters that "need escaping only when noglob is not present". (Neither (q) nor (b) do this since both of them escape backslashes.) Cheers, Daniel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: _libvirt, $opt_args, and noglob 2016-09-04 18:26 _libvirt, $opt_args, and noglob Daniel Shahaf @ 2016-09-06 23:46 ` Daniel Shahaf 2016-09-07 9:53 ` Marko Myllynen 0 siblings, 1 reply; 4+ messages in thread From: Daniel Shahaf @ 2016-09-06 23:46 UTC (permalink / raw) To: zsh-workers; +Cc: Marko Myllynen Daniel Shahaf wrote on Sun, Sep 04, 2016 at 18:26:22 +0000: > In the special case of _libvirt, simply adding noglob to the > _call_program arguments would probably fix this. Done, but the last line touches the sudo invocation from Oliver's gain-privileges patch. Oliver: shall I wait with committing this until you've committed gain-privileges? [[[ diff --git a/Completion/Unix/Command/_libvirt b/Completion/Unix/Command/_libvirt index 658e197..2a0a34c 100644 --- a/Completion/Unix/Command/_libvirt +++ b/Completion/Unix/Command/_libvirt @@ -140,51 +140,51 @@ case $state in local -a values case $words[CURRENT-1] in --domain) - values=( $(_call_program domains "virsh $conn_opt list ${dom_opts[$cmd]:-"--all"} --name") ) + values=( $(_call_program domains "noglob 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 $conn_opt iface-list ${iface_opts[$cmd]:-"--all"}")"}/ Name*/}:#---*}/ */} ) + values=( ${${${${(f):-"$(_call_program interfaces "noglob 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 $conn_opt net-list ${net_opts[$cmd]:-"--all"} --name") ) + values=( $(_call_program networks "noglob 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 $conn_opt nodedev-list") ) + values; values=( $(_call_program nodedevs "noglob virsh $conn_opt nodedev-list") ) [[ -n $values ]] && _wanted devices expl device compadd ${=values} && return 0 return 1 ;; --nwfilter) - values=( ${${${${(f):-"$(_call_program nwfilters "virsh $conn_opt nwfilter-list")"}/ UUID*/}:#---*}/ */} ) + values=( ${${${${(f):-"$(_call_program nwfilters "noglob 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 $conn_opt pool-list ${pool_opts[$cmd]:-"--all"}")"}/ Name*/}:#---*}/ */} ) + values=( ${${${${(f):-"$(_call_program pools "noglob 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 $conn_opt secret-list")"}/ UUID*/}:#---*}/ */} ) + values=( ${${${${(f):-"$(_call_program secrets "noglob 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]} > 0 )) && dom=${words[1+${(k)words[(I)--domain]}]} [[ -z $dom ]] && return 1 - values=( ${${${${(f):-"$(_call_program snapshots "virsh $conn_opt snapshot-list --domain ${(q)dom} 2>/dev/null")"}/ Name*/}:#---*}/ */} ) + values=( ${${${${(f):-"$(_call_program snapshots "noglob 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]} > 0 )) && pool=${words[1+${(k)words[(I)--pool]}]} [[ -z $pool ]] && return 1 - values=( ${${${${(f):-"$(_call_program volumes "virsh $conn_opt vol-list --pool ${(q)pool} 2>/dev/null")"}/ Name*/}:#---*}/ */} ) + values=( ${${${${(f):-"$(_call_program volumes "noglob virsh $conn_opt vol-list --pool ${(q)pool} 2>/dev/null")"}/ Name*/}:#---*}/ */} ) [[ -n $values ]] && _wanted volumes expl volume compadd ${=values} && return 0 return 1 ;; @@ -199,7 +199,7 @@ case $state in # Allow passing domain without --domain with few of the most used commands if [[ $cmd == (destroy|reboot|reset|start|shutdown) ]]; then if [[ $words[CURRENT-1] == $cmd ]]; then - values=( $(_call_program domains "virsh $conn_opt list ${dom_opts[$cmd]:-"--all"} --name") ) + values=( $(_call_program domains "noglob virsh $conn_opt list ${dom_opts[$cmd]:-"--all"} --name") ) [[ -n $values ]] && _wanted domains expl domain compadd ${=values} && return 0 fi fi @@ -224,7 +224,7 @@ case $state in local srv ; (( ${(k)words[(I)--server]} > 0 )) && srv=${words[1+${(k)words[(I)--server]}]} [[ -z $srv ]] && return 1 [[ -n ${srv//[[:alnum:]]} ]] && return 1 - _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 + _wanted clients expl client compadd ${=${${(f):-"$(noglob 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]*} ]]] > This might not work for arbitrary other callsites, though. I think > the general fix would be to backslash-escape the characters that "need > escaping only when noglob is not present". (Neither (q) nor (b) do > this since both of them escape backslashes.) Leaving this for Future Work. Cheers, Daniel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: _libvirt, $opt_args, and noglob 2016-09-06 23:46 ` Daniel Shahaf @ 2016-09-07 9:53 ` Marko Myllynen 2016-09-07 22:42 ` Daniel Shahaf 0 siblings, 1 reply; 4+ messages in thread From: Marko Myllynen @ 2016-09-07 9:53 UTC (permalink / raw) To: Daniel Shahaf, zsh-workers Hi, On 2016-09-07 02:46, Daniel Shahaf wrote: > Daniel Shahaf wrote on Sun, Sep 04, 2016 at 18:26:22 +0000: >> In the special case of _libvirt, simply adding noglob to the >> _call_program arguments would probably fix this. > > Done, but the last line touches the sudo invocation from Oliver's > gain-privileges patch. Oliver: shall I wait with committing this until > you've committed gain-privileges? > > @@ -224,7 +224,7 @@ case $state in > local srv ; (( ${(k)words[(I)--server]} > 0 )) && srv=${words[1+${(k)words[(I)--server]}]} > [[ -z $srv ]] && return 1 > [[ -n ${srv//[[:alnum:]]} ]] && return 1 > - _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 > + _wanted clients expl client compadd ${=${${(f):-"$(noglob 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]*} > ]]] Do we need any of (Q)'s there any more after your addition of: uri=${uri//(#m)\\([\\:])/${MATCH[2]}} # opt_args elements are colon-escaped Based on a quick test looks like they could be dropped (or perhaps even changed to (q))? Thanks, -- Marko Myllynen ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: _libvirt, $opt_args, and noglob 2016-09-07 9:53 ` Marko Myllynen @ 2016-09-07 22:42 ` Daniel Shahaf 0 siblings, 0 replies; 4+ messages in thread From: Daniel Shahaf @ 2016-09-07 22:42 UTC (permalink / raw) To: Marko Myllynen; +Cc: zsh-workers See the penultimate paragraph for a question about whether ${(q)} should quote leading '=' signs to guard against the EQUALS option. Marko Myllynen wrote on Wed, Sep 07, 2016 at 12:53:13 +0300: > Hi, > > On 2016-09-07 02:46, Daniel Shahaf wrote: > > Daniel Shahaf wrote on Sun, Sep 04, 2016 at 18:26:22 +0000: > >> In the special case of _libvirt, simply adding noglob to the > >> _call_program arguments would probably fix this. > > > > Done, but the last line touches the sudo invocation from Oliver's > > gain-privileges patch. Oliver: shall I wait with committing this until > > you've committed gain-privileges? > > > > @@ -224,7 +224,7 @@ case $state in > > local srv ; (( ${(k)words[(I)--server]} > 0 )) && srv=${words[1+${(k)words[(I)--server]}]} > > [[ -z $srv ]] && return 1 > > [[ -n ${srv//[[:alnum:]]} ]] && return 1 > > - _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 > > + _wanted clients expl client compadd ${=${${(f):-"$(noglob 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]*} > > ]]] > > Do we need any of (Q)'s there any more after your addition of: > > uri=${uri//(#m)\\([\\:])/${MATCH[2]}} # opt_args elements are colon-escaped > > Based on a quick test looks like they could be dropped (or > perhaps even changed to (q))? _call_program expects its argument to be (q)'d. (The interface is like 'eval', not like execve(2).) The values of $opt_args are *already* (q)'d. If the command line is «virsh -c foo\\bar» then the value of $pot_args[-c] includes two backslashes, not one as virsh(1)'s argv[2] would see.¹ Therefore, I think the correct way to pass values derived from $opt_args — this includes $conn_opt — to _call_program is to use neither (q) nor (Q). That is the general answer. For the specific case of $conn_opt, the URI format validator ensures it has nothing that needs quoting, apart from '?' and '=', and the patch in the grandparent message removes the need for '?' to be quoted. However, there's no easy to to quote '=' when it's the first character of a word, and _call_program does honour the EQUALS option: . % _f() { compadd - $(_call_program x 'echo =true') } % f <TAB> <becomes> % f /bin/true Again, this is harmless as far as _libvirt is concerned, but it might not be so in other callsites. I think the right general solution to this would be to write ${(q)${(Q)conn_opts}} in the _call_program invocation, and teach the (q) flag to escape leading equals signs. Anyway, back to Marko's question: I assume even if the EQUALS option did affect the command executed, that wouldn't cause any harm, so I think the primary reason to get _libvirt's quoting right is in case somebody ever uses _libvirt as a basis for some other completion function. That's part of why I committed the (b) patch, too. Cheers, Daniel ¹ This assumes that the word being completed, $CURRENT, is not the argument to -c, in which cae $opt_args[-c] could have an opening quote but no matching closing quote. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-07 22:43 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-04 18:26 _libvirt, $opt_args, and noglob Daniel Shahaf 2016-09-06 23:46 ` Daniel Shahaf 2016-09-07 9:53 ` Marko Myllynen 2016-09-07 22:42 ` Daniel Shahaf
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).