zsh virsh completion

zsh virsh completion

[Marko Myllynen]

Hi,

Below is a patch to implement basic completion for the virsh(1) [1]
command from libvirt [2], it completes all virsh commands and their
respective options.

I think it's pretty good and hopefully the caching approach is sane (of
course it'd be nice to get rid of that awk call but no biggie).

Two somewhat questions that came to my mind:

- virsh help <command> output is pretty much like --help output of any
other program, is there a trick to make the option descriptions also
available when completing virsh command options?

- would it make sense (or is it perhaps already possible somehow) to
optionally enable "best guess" completion for all commands which have
no command specific completion rules available yet? There are lots of
commands for which completing just the options captured from --help
output would already be hugely helpful. With some other commands (e.g.,
git) that would fall flat but for many smaller / trivial commands that
might be just well enough. (If there's this kind of feature already
available, then I've just missed that.)

Anyway, here's the patch, please apply if it looks good.

1) https://www.mankier.com/1/virsh
2) https://libvirt.org/

---
 Completion/Unix/Command/_virsh | 50 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Completion/Unix/Command/_virsh

diff --git a/Completion/Unix/Command/_virsh b/Completion/Unix/Command/_virsh
new file mode 100644
index 0000000..179ae86
--- /dev/null
+++ b/Completion/Unix/Command/_virsh
@@ -0,0 +1,50 @@
+#compdef virsh
+
+local curcontext="$curcontext" state expl ret=1
+
+_arguments -A "-*" -C -S -s -w \
+  '(- *)'{-h,--help}'[print help information and exit]' \
+  '(- *)'{-v,--version=short}'[print short version information and exit]' \
+  '(- *)'{-V,--version=long}'[print long version information and exit]' \
+  '(-c --connect)'{-c+,--connect}'[specify connection URI]:URI:_hosts' \
+  '(-d --debug)'{-d+,--debug}'[set debug level]:level:(0 1 2 3 4)' \
+  '(-e --escape)'{-e+,--escape}'[set escape sequence for console]:sequence' \
+  '(-k --keepalive-interval)'{-k+,--keepalive-interval}'[set keepalive interval]:interval' \
+  '(-K --keepalive-count)'{-K+,--keepalive-count}'[set keepalive count]:count' \
+  '(-l --log)'{-l+,--log}'[specify log file]:file:_files' \
+  '(-q --quiet)'{-q,--quiet}'[quiet mode]' \
+  '(-r --readonly)'{-r,--readonly}'[connect readonly]' \
+  '(-t --timing)'{-t,--timing}'[print timing information]' \
+  '1:command:->commands' \
+  '*:cmdopt:->cmdopts' \
+  && return 0
+
+# 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=cmdopts
+
+if (( ! $+_cache_virsh_cmds )); then
+  _cache_virsh_cmds=( ${="$(virsh help 2>&1 | awk '!/:/ {print $1}')"} )
+fi
+if (( ! $+_cache_virsh_cmdopts )); then
+  typeset -gA _cache_virsh_cmdopts
+fi
+
+case $state in
+  commands)
+    _wanted commands expl 'virsh command' compadd -a _cache_virsh_cmds && ret=0
+  ;;
+  cmdopts)
+    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)"$(virsh help $cmd 2>&1)"}}/\[}/\]}/\;}:#-[-0-9A-Za-z]*}
+    fi
+    _values -w options ${=_cache_virsh_cmdopts[$cmd]} && ret=0
+  ;;
+esac
+
+return ret

Thanks,

-- 
Marko Myllynen

Re: zsh virsh completion

[Roman Neuhauser]

# myllynen@redhat.com / 2016-07-11 14:52:02 +0300:
> I think it's pretty good and hopefully the caching approach is sane (of
> course it'd be nice to get rid of that awk call but no biggie).

> +  _cache_virsh_cmds=( ${="$(virsh help 2>&1 | awk '!/:/ {print $1}')"} )

${${${${(f):-"$(virsh help)"}:#*:}/# ##}/ *}

-- 
roman

Re: zsh virsh completion

[Bart Schaefer]

On Jul 11,  2:52pm, Marko Myllynen wrote:
}
} - virsh help <command> output is pretty much like --help output of any
} other program, is there a trick to make the option descriptions also
} available when completing virsh command options?

I think you're asking about the call to _values in the cmdopts state
handler.  Each of the option specs (the words that you're caching in
_cache_virsh_cmdopts) can be in the form "-option[description]" just
like an option spec for _arguments.  So you would need to parse the
output of $(virsh help $cmd) a little more deeply than you presently
do to build those strings, and then do something a bit more clever
than ${=_cache_virsh_cmdopts} to cache and split them.

There is support in _arguments for parsing --help formatted output.
The basic pattern for having _arguments do this when "virsh --help"
doesn't produce that output, is to add a "command" style similar to:

  zstyle :completion::complete:virsh::options command 'virsh help'

The exact zstyle context to use depends on how you're making the call
to _arguments.  For your virsh completer you would need another call
to _arguments in the cmdopts state handler and corresponding change
to the context.  This can be tricky to get right.

} - would it make sense (or is it perhaps already possible somehow) to
} optionally enable "best guess" completion for all commands which have
} no command specific completion rules available yet? There are lots of
} commands for which completing just the options captured from --help
} output would already be hugely helpful.

I think you're looking for

    compdef -P '*' _gnu_generic

Which just says to try GNU-style options after all else fails, no
matter what the command word might be.  However, here's presently no
way to make sure this is called after other "compdef -P" patterns
that may be more specific.

(This leads me to wonder whether all those "#compdef -P" lines in
Completion/** files ought to instead be "#compdef -p".  I guess the
point is to allow the user to override with other compdefs and try
the supplied completions if none such is present?  But in that
case shouldn't _init_d also use -P rather than -p ?)

Re: zsh virsh completion

[Oliver Kiddle]

Marko Myllynen wrote:
> Below is a patch to implement basic completion for the virsh(1) [1]
> command from libvirt [2], it completes all virsh commands and their
> respective options.

The function should probably be named _libvirt then - in much the same
way as we use e.g. _subversion for svn.

> - virsh help <command> output is pretty much like --help output of any
> other program, is there a trick to make the option descriptions also
> available when completing virsh command options?

If you don't use _arguments --, you can parse the output manually.

> - would it make sense (or is it perhaps already possible somehow) to
> optionally enable "best guess" completion for all commands which have
> no command specific completion rules available yet? There are lots of

There are commands where running the command with --help/-h or
whatever could do something destructive. I forget which command it
was but I remember years ago finding that a completion was spewing
messages into syslog. If it was going to be a catch-all default,
it might be safer to use man pages as the source.

> +_arguments -A "-*" -C -S -s -w \

-w, really? That's rare.

> +  _cache_virsh_cmds=( ${="$(virsh help 2>&1 | awk '!/:/ {print $1}')"} )

You should use _call_program as a wrapper around virsh.

> +    _values -w options ${=_cache_virsh_cmdopts[$cmd]} && ret=0

I'm not convinced that _values is gaining you anything over a plain
compadd (with _wanted). I wouldn't typically use it with generated
matches. If you want the duplicate removal, use -F words

With this in place there's no filename completion which can be quite
annoying if you need to complete files. So unless virsh sub-commands
never take filenames as arguments, I'd have it fallback to _default.
In this case, using _alternative.  Also, for option completion, it
should check the prefix-needed style.

Oliver

Re: zsh virsh completion

[Marko Myllynen]

Hi,

On 2016-07-12 01:07, Oliver Kiddle wrote:
> Marko Myllynen wrote:
>> Below is a patch to implement basic completion for the virsh(1) [1]
>> command from libvirt [2], it completes all virsh commands and their
>> respective options.
> 
> The function should probably be named _libvirt then - in much the same
> way as we use e.g. _subversion for svn.

Ok, changed.

>> - virsh help <command> output is pretty much like --help output of any
>> other program, is there a trick to make the option descriptions also
>> available when completing virsh command options?
> 
> If you don't use _arguments --, you can parse the output manually.

The options are pretty self-describing so description for them might be
an additional improvement in the future, based on the comment from Bart
it sounded this might be tricky to get right.

>> - would it make sense (or is it perhaps already possible somehow) to
>> optionally enable "best guess" completion for all commands which have
>> no command specific completion rules available yet? There are lots of
> 
> There are commands where running the command with --help/-h or
> whatever could do something destructive. I forget which command it
> was but I remember years ago finding that a completion was spewing
> messages into syslog. If it was going to be a catch-all default,
> it might be safer to use man pages as the source.

Interesting, thanks for the background info.

>> +_arguments -A "-*" -C -S -s -w \
> 
> -w, really? That's rare.

Yes, virsh accepts e.g. -r -d 0, -r -d0, -rd 0, or -rd0.

>> +  _cache_virsh_cmds=( ${="$(virsh help 2>&1 | awk '!/:/ {print $1}')"} )
> 
> You should use _call_program as a wrapper around virsh.

Ok, using virsh as the tag.

>> +    _values -w options ${=_cache_virsh_cmdopts[$cmd]} && ret=0
> 
> I'm not convinced that _values is gaining you anything over a plain
> compadd (with _wanted). I wouldn't typically use it with generated
> matches. If you want the duplicate removal, use -F words

I started with _wanted/compadd but couldn't get it working and since
_values did the trick I was happy with.

> With this in place there's no filename completion which can be quite
> annoying if you need to complete files. So unless virsh sub-commands
> never take filenames as arguments, I'd have it fallback to _default.
> In this case, using _alternative.  Also, for option completion, it
> should check the prefix-needed style.

Good point on filenames. There are 227 virsh commands (as of libvirt
2.0), I haven't gone thru them all, but some of them have options which
take file names as arguments while others don't. I'm not sure what
would be the most optimal approach here, I now added a _default
fallback in case the user types something starting with / or ./.

Below is the updated patch, hopefully it addresses most of the feedback
properly.

---
 Completion/Unix/Command/_libvirt | 59 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Completion/Unix/Command/_libvirt

diff --git a/Completion/Unix/Command/_libvirt b/Completion/Unix/Command/_libvirt
new file mode 100644
index 0000000..ac19ac3
--- /dev/null
+++ b/Completion/Unix/Command/_libvirt
@@ -0,0 +1,59 @@
+#compdef virsh
+
+_libvirt () {
+  local curcontext="$curcontext" state line expl ret=1
+
+  case $service in
+  virsh)
+    if (( ! $+_cache_virsh_cmds )); then
+      _cache_virsh_cmds=( ${="$(_call_program virsh virsh help 2>&1 | awk '!/:/ {print $1}')"} )
+    fi
+    if (( ! $+_cache_virsh_cmdopts )); then
+      typeset -gA _cache_virsh_cmdopts
+    fi
+    _arguments -A "-*" -C -S -s -w \
+      '(- *)'{-h,--help}'[print help information and exit]' \
+      '(- *)'{-v,--version=short}'[print short version information and exit]' \
+      '(- *)'{-V,--version=long}'[print long version information and exit]' \
+      '(-c --connect)'{-c+,--connect}'[specify connection URI]:URI:_hosts' \
+      '(-d --debug)'{-d+,--debug}'[set debug level]:level:(0 1 2 3 4)' \
+      '(-e --escape)'{-e+,--escape}'[set escape sequence for console]:sequence' \
+      '(-k --keepalive-interval)'{-k+,--keepalive-interval}'[set keepalive interval]:interval' \
+      '(-K --keepalive-count)'{-K+,--keepalive-count}'[set keepalive count]:count' \
+      '(-l --log)'{-l+,--log}'[specify log file]:file:_files' \
+      '(-q --quiet)'{-q,--quiet}'[quiet mode]' \
+      '(-r --readonly)'{-r,--readonly}'[connect readonly]' \
+      '(-t --timing)'{-t,--timing}'[print timing information]' \
+      '1:command:->virsh_cmds' \
+      '*:cmdopt:->virsh_cmdopts' \
+      && return 0
+      # 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
+  ;;
+  esac
+
+  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
+        _default && return 0
+      fi
+      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]*}
+      fi
+      _values -w options ${=_cache_virsh_cmdopts[$cmd]} && ret=0
+    ;;
+  esac
+
+  return ret
+}
+
+_libvirt "$@"

Thanks,

-- 
Marko Myllynen

Re: zsh virsh completion

[Daniel Shahaf]

Marko Myllynen wrote on Mon, Jul 11, 2016 at 14:52:02 +0300:
> Below is a patch to implement basic completion for the virsh(1) [1]
> command from libvirt [2], it completes all virsh commands and their
> respective options.
> 
> I think it's pretty good and hopefully the caching approach is sane (of
> course it'd be nice to get rid of that awk call but no biggie).

Thanks!

Note there are two or three other _virsh's floating around:

https://github.com/jplitza/zsh-virsh-autocomplete/blob/master/_virsh
https://github.com/zsh-users/zsh-completions/blob/master/src/_virsh
https://github.com/Aso23/zsh_virsh_autocompletion/blob/master/_virsh

Those seem to have features your patch lacks, e.g., completing domain
(VM name) arguments.

What can we do about this duplication?

Cheers,

Daniel

Re: zsh virsh completion

[Marko Myllynen]

Hi,

On 2016-07-13 07:59, Daniel Shahaf wrote:
> Marko Myllynen wrote on Mon, Jul 11, 2016 at 14:52:02 +0300:
>> Below is a patch to implement basic completion for the virsh(1) [1]
>> command from libvirt [2], it completes all virsh commands and their
>> respective options.
> 
> Note there are two or three other _virsh's floating around:
> 
> https://github.com/jplitza/zsh-virsh-autocomplete/blob/master/_virsh
> https://github.com/zsh-users/zsh-completions/blob/master/src/_virsh
> https://github.com/Aso23/zsh_virsh_autocompletion/blob/master/_virsh

I wasn't aware of these. The last one on the list seems to be the first
incarnation, the middle one adds very little to it, the first link
provides the most recent version.

Those repos seem to have some other goodies as well, it's a pity that
they haven't been upstreamed. Many of us work with several systems and
downloading something for each is inconvenient, sometimes even
impossible, so if something is not in upstream and in distro packages,
it's almost like it didn't exist at all, unfortunately.

> Those seem to have features your patch lacks, e.g., completing domain
> (VM name) arguments.

Domain completion is certainly nice, however option completion seems to
be broken (try e.g. virsh perf <tab>) and all lack at least storage
pool and network completions.

Oliver cleaned up and committed my earlier patch, here's an update for
it, this patch implements completion for the rest of the commands that
are part of libvirt (as is with libvirt-2.0 RHEL 7 packages). Few of
them barely deserve completion but are there for completeness sake.
(No virt-admin command accepts filenames so those are not offered.)

---
 Completion/Unix/Command/_libvirt | 89 ++++++++++++++++++++++++++++++++++------
 1 file changed, 76 insertions(+), 13 deletions(-)

diff --git a/Completion/Unix/Command/_libvirt b/Completion/Unix/Command/_libvirt
index a9249b3..2053d15 100644
--- a/Completion/Unix/Command/_libvirt
+++ b/Completion/Unix/Command/_libvirt
@@ -1,34 +1,82 @@
-#compdef virsh
+#compdef virsh virt-admin virt-host-validate virt-pki-validate virt-xml-validate
 
 local curcontext="$curcontext" state line expl ret=1
 
+local exargs="-h --help -V -v --version=short --version=long"
+local -a common_opts interact_cmds
+common_opts=(
+  '(- *)'{-h,--help}'[print help information and exit]'
+  '(- *)'{-v,--version=short}'[print short version information and exit]'
+  '(- *)'{-V,--version=long}'[print long version information and exit]'
+  "(-c --connect $exargs)"{-c+,--connect}'[specify connection URI]:URI:_hosts'
+  "(-d --debug -q --quiet $exargs)"{-d+,--debug}'[set debug level]:level:(0 1 2 3 4)'
+  "(-l --log $exargs)"{-l+,--log}'[specify log file]:file:_files'
+  "(-q --quiet -d --debug $exargs)"{-q,--quiet}'[quiet mode]'
+)
+interact_cmds=(cd echo exit quit connect)
+
 case $service in
   virsh)
     if (( ! $+_cache_virsh_cmds )); then
       _cache_virsh_cmds=( ${${${${(f):-"$(_call_program options virsh help)"}:#*:}/# ##}/ *} )
+      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
     fi
     _arguments -A "-*" -C -S -s -w \
-      '(- *)'{-h,--help}'[print help information and exit]' \
-      '(- *)'{-v,--version=short}'[print short version information and exit]' \
-      '(- *)'{-V,--version=long}'[print long version information and exit]' \
-      '(-c --connect)'{-c+,--connect}'[specify connection URI]:URI:_hosts' \
-      '(-d --debug)'{-d+,--debug}'[set debug level]:level:(0 1 2 3 4)' \
-      '(-e --escape)'{-e+,--escape}'[set escape sequence for console]:sequence' \
-      '(-k --keepalive-interval)'{-k+,--keepalive-interval}'[set keepalive interval]:interval' \
-      '(-K --keepalive-count)'{-K+,--keepalive-count}'[set keepalive count]:count' \
-      '(-l --log)'{-l+,--log}'[specify log file]:file:_files' \
-      '(-q --quiet)'{-q,--quiet}'[quiet mode]' \
-      '(-r --readonly)'{-r,--readonly}'[connect readonly]' \
-      '(-t --timing)'{-t,--timing}'[print timing information]' \
+      "$common_opts[@]" \
+      "(-e --escape $exargs)"{-e+,--escape}'[set escape sequence for console]:sequence' \
+      "(-k --keepalive-interval $exargs)"{-k+,--keepalive-interval}'[set keepalive interval]:interval' \
+      "(-K --keepalive-count $exargs)"{-K+,--keepalive-count}'[set keepalive count]:count' \
+      "(-r --readonly $exargs)"{-r,--readonly}'[connect readonly]' \
+      "(-t --timing $exargs)"{-t,--timing}'[print timing information]' \
       '1:command:->virsh_cmds' \
       '*:cmdopt:->virsh_cmdopts' && 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
   ;;
+  virt-admin)
+    if (( ! $+_cache_virt_admin_cmds )); then
+      _cache_virt_admin_cmds=( ${${${${(f):-"$(_call_program options virt-admin help)"}:#*:}/# ##}/ *} )
+      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
+    fi
+    _arguments -A "-*" -C -S -s -w \
+      "$common_opts[@]" \
+      '1:command:->virt_admin_cmds' \
+      '*:cmdopt:->virt_admin_cmdopts' && return
+      # Same as with virsh above
+      [[ -z $state ]] && state=virt_admin_cmdopts
+  ;;
+  virt-host-validate)
+    _arguments -A "-*" -S \
+      '(- *)'{-h,--help}'[print help information and exit]' \
+      '(- *)'{-v,--version}'[print version information and exit]' \
+      '(- *)'{-q,--quiet}'[quiet mode]' \
+      '1:hv-type:(qemu lxc)' && return
+  ;;
+  virt-pki-validate)
+    _arguments -A "-*" -S \
+      '(- *)'{-h,--help}'[print help information and exit]' \
+      '(- *)'{-V,--version}'[print version information and exit]' \
+      && return
+  ;;
+  virt-xml-validate)
+    _arguments -A "-*" -S \
+      '(- *)'{-h,--help}'[print help information and exit]' \
+      '(- *)'{-V,--version}'[print version information and exit]' \
+      '1:file:_files -g "*.xml(-.)"' \
+      '2:schema:(domainsnapshot domain network storagepool storagevol nodedev capability nwfilter secret interface)' \
+      && return
+  ;;
 esac
 
 case $state in
@@ -50,6 +98,21 @@ case $state in
     fi
     _values -w options ${=_cache_virsh_cmdopts[$cmd]} && ret=0
   ;;
+  virt_admin_cmds)
+    _wanted commands expl 'virt-admin command' compadd -a _cache_virt_admin_cmds && ret=0
+  ;;
+  virt_admin_cmdopts)
+    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 [[ -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]*}
+    fi
+    _values -w options ${=_cache_virt_admin_cmdopts[$cmd]} && ret=0
+  ;;
+
 esac
 
 return ret

Thanks,

-- 
Marko Myllynen

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Daniel Shahaf]

Resubjecting to draw attention to your patch.

I haven't done a full review, but I did spot a couple of trivial issues:

Marko Myllynen wrote on Mon, Jul 18, 2016 at 15:06:52 +0300:
>  case $service in
>    virsh)
>      if (( ! $+_cache_virsh_cmds )); then
>        _cache_virsh_cmds=( ${${${${(f):-"$(_call_program options virsh help)"}:#*:}/# ##}/ *} )
> +      for icmd in $interact_cmds; do

Need to declare $icmd local.

> +        _cache_virsh_cmds[$_cache_virsh_cmds[(i)$icmd]]=()
> +      done
>      fi
>      if (( ! $+_cache_virsh_cmdopts )); then
>        typeset -gA _cache_virsh_cmdopts
>      fi
>      _arguments -A "-*" -C -S -s -w \

Oliver remarked on the -w earlier, are you quite sure it's correct?

              -w     In combination with -s, allow option stacking even if one
                     or more of the options take arguments.  For example, if
                     -x takes an argument, with no -s, `-xy' is considered as
                     a single (unhandled) option; with -s, -xy is an option
                     with the argument `y'; with both -s and -w, -xy may be
                     the option -x and the option -y with arguments still to
                     come.

(-s and -w are options to _arguments, -xy is a word on the command line
being completed)

> -      '(-c --connect)'{-c+,--connect}'[specify connection URI]:URI:_hosts' \

Should be {-c+,--connect=} with an equals sign?

Cheers,

Daniel

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Marko Myllynen]

Hi,

On 2016-07-20 09:58, Daniel Shahaf wrote:
> Resubjecting to draw attention to your patch.
> 
> I haven't done a full review, but I did spot a couple of trivial issues:
> 
> Marko Myllynen wrote on Mon, Jul 18, 2016 at 15:06:52 +0300:
>>  case $service in
>>    virsh)
>>      if (( ! $+_cache_virsh_cmds )); then
>>        _cache_virsh_cmds=( ${${${${(f):-"$(_call_program options virsh help)"}:#*:}/# ##}/ *} )
>> +      for icmd in $interact_cmds; do
> 
> Need to declare $icmd local.

Good catch.

>> +        _cache_virsh_cmds[$_cache_virsh_cmds[(i)$icmd]]=()
>> +      done
>>      fi
>>      if (( ! $+_cache_virsh_cmdopts )); then
>>        typeset -gA _cache_virsh_cmdopts
>>      fi
>>      _arguments -A "-*" -C -S -s -w \
> 
> Oliver remarked on the -w earlier, are you quite sure it's correct?
> 
>               -w     In combination with -s, allow option stacking even if one
>                      or more of the options take arguments.  For example, if
>                      -x takes an argument, with no -s, `-xy' is considered as
>                      a single (unhandled) option; with -s, -xy is an option
>                      with the argument `y'; with both -s and -w, -xy may be
>                      the option -x and the option -y with arguments still to
>                      come.
> 
> (-s and -w are options to _arguments, -xy is a word on the command line
> being completed)

I mentioned that virsh accepts all the following: -r -d 0, -r -d0, -rd
0, or -rd0. So to me it would seem that -s -w is correct here, right?

>> -      '(-c --connect)'{-c+,--connect}'[specify connection URI]:URI:_hosts' \
> 
> Should be {-c+,--connect=} with an equals sign?

Hmm, perhaps that's indeed better, the virsh(1) man page doesn't use
equal sign and arguments with or without it accepted.

Below is an updated patch against git master which also completes
virt-admin(1) completion: it now completes client/server IDs/names
where needed. If the approach looks viable, perhaps we could use
something similar later on when completing domains/etc with virsh?

There's one obvious case I'm not sure how to best handle it: sudo use.
virt-admin uses a (typically) root-owned socket so without privileges
you can't get the list of clients/servers. Of course, if sudo starts to
ask password then the end result is not pretty. OTOH, if you don't have
sudo or are not root then using virt-admin is not possible. Ideas how
to do this in the most elegant fashion are most welcome.

(Please note that I will be offline for the next few weeks so I will
get back this once I'm back online again.)

---
 Completion/Unix/Command/_libvirt | 103 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 90 insertions(+), 13 deletions(-)

diff --git a/Completion/Unix/Command/_libvirt b/Completion/Unix/Command/_libvirt
index a9249b3..c855ac9 100644
--- a/Completion/Unix/Command/_libvirt
+++ b/Completion/Unix/Command/_libvirt
@@ -1,34 +1,84 @@
-#compdef virsh
+#compdef virsh virt-admin virt-host-validate virt-pki-validate virt-xml-validate
 
 local curcontext="$curcontext" state line expl ret=1
 
+local exargs="-h --help -V -v --version=short --version=long"
+local -a common_opts interact_cmds
+common_opts=(
+  '(- *)'{-h,--help}'[print help information and exit]'
+  '(- *)'{-v,--version=short}'[print short version information and exit]'
+  '(- *)'{-V,--version=long}'[print long version information and exit]'
+  "(-c --connect $exargs)"{-c+,--connect=}'[specify connection URI]:URI:_hosts'
+  "(-d --debug -q --quiet $exargs)"{-d+,--debug=}'[set debug level]:level:(0 1 2 3 4)'
+  "(-l --log $exargs)"{-l+,--log=}'[specify log file]:file:_files'
+  "(-q --quiet -d --debug $exargs)"{-q,--quiet}'[quiet mode]'
+)
+interact_cmds=(cd echo exit quit connect)
+
 case $service in
   virsh)
     if (( ! $+_cache_virsh_cmds )); then
       _cache_virsh_cmds=( ${${${${(f):-"$(_call_program options 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
     fi
     _arguments -A "-*" -C -S -s -w \
-      '(- *)'{-h,--help}'[print help information and exit]' \
-      '(- *)'{-v,--version=short}'[print short version information and exit]' \
-      '(- *)'{-V,--version=long}'[print long version information and exit]' \
-      '(-c --connect)'{-c+,--connect}'[specify connection URI]:URI:_hosts' \
-      '(-d --debug)'{-d+,--debug}'[set debug level]:level:(0 1 2 3 4)' \
-      '(-e --escape)'{-e+,--escape}'[set escape sequence for console]:sequence' \
-      '(-k --keepalive-interval)'{-k+,--keepalive-interval}'[set keepalive interval]:interval' \
-      '(-K --keepalive-count)'{-K+,--keepalive-count}'[set keepalive count]:count' \
-      '(-l --log)'{-l+,--log}'[specify log file]:file:_files' \
-      '(-q --quiet)'{-q,--quiet}'[quiet mode]' \
-      '(-r --readonly)'{-r,--readonly}'[connect readonly]' \
-      '(-t --timing)'{-t,--timing}'[print timing information]' \
+      "$common_opts[@]" \
+      "(-e --escape $exargs)"{-e+,--escape=}'[set escape sequence for console]:sequence' \
+      "(-k --keepalive-interval $exargs)"{-k+,--keepalive-interval=}'[set keepalive interval]:interval' \
+      "(-K --keepalive-count $exargs)"{-K+,--keepalive-count=}'[set keepalive count]:count' \
+      "(-r --readonly $exargs)"{-r,--readonly}'[connect readonly]' \
+      "(-t --timing $exargs)"{-t,--timing}'[print timing information]' \
       '1:command:->virsh_cmds' \
       '*:cmdopt:->virsh_cmdopts' && 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
   ;;
+  virt-admin)
+    if (( ! $+_cache_virt_admin_cmds )); then
+      _cache_virt_admin_cmds=( ${${${${(f):-"$(_call_program options 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
+    fi
+    _arguments -A "-*" -C -S -s -w \
+      "$common_opts[@]" \
+      '1:command:->virt_admin_cmds' \
+      '*:cmdopt:->virt_admin_cmdopts' && return
+      # Same as with virsh above
+      [[ -z $state ]] && state=virt_admin_cmdopts
+  ;;
+  virt-host-validate)
+    _arguments -A "-*" -S \
+      '(- *)'{-h,--help}'[print help information and exit]' \
+      '(- *)'{-v,--version}'[print version information and exit]' \
+      '(- *)'{-q,--quiet}'[quiet mode]' \
+      '1:hv-type:(qemu lxc)' && return
+  ;;
+  virt-pki-validate)
+    _arguments -A "-*" -S \
+      '(- *)'{-h,--help}'[print help information and exit]' \
+      '(- *)'{-V,--version}'[print version information and exit]' \
+      && return
+  ;;
+  virt-xml-validate)
+    _arguments -A "-*" -S \
+      '(- *)'{-h,--help}'[print help information and exit]' \
+      '(- *)'{-V,--version}'[print version information and exit]' \
+      '1:file:_files -g "*.xml(-.)"' \
+      '2:schema:(domainsnapshot domain network storagepool storagevol nodedev capability nwfilter secret interface)' \
+      && return
+  ;;
 esac
 
 case $state in
@@ -50,6 +100,33 @@ case $state in
     fi
     _values -w options ${=_cache_virsh_cmdopts[$cmd]} && ret=0
   ;;
+  virt_admin_cmds)
+    _wanted commands expl 'virt-admin command' compadd -a _cache_virt_admin_cmds && ret=0
+  ;;
+  virt_admin_cmdopts)
+    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
+    fi
+    if [[ $words[-2] == --client ]]; then
+      local srv
+      for (( i = 2; i <= $#words; i++ )); do
+        [[ $words[$i] == --server ]] && srv=$words[$i+1] && break
+      done
+      [[ -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]*}
+    fi
+    [[ -n $_cache_virt_admin_cmdopts[$cmd] ]] && \
+      _values -w options ${=_cache_virt_admin_cmdopts[$cmd]} && ret=0
+  ;;
+
 esac
 
 return ret


Thanks,

-- 
Marko Myllynen

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Daniel Shahaf]

Marko Myllynen wrote on Wed, Jul 20, 2016 at 11:36:36 +0300:
> On 2016-07-20 09:58, Daniel Shahaf wrote:
> > Marko Myllynen wrote on Mon, Jul 18, 2016 at 15:06:52 +0300:
> >> +        _cache_virsh_cmds[$_cache_virsh_cmds[(i)$icmd]]=()
> >> +      done
> >>      fi
> >>      if (( ! $+_cache_virsh_cmdopts )); then
> >>        typeset -gA _cache_virsh_cmdopts
> >>      fi
> >>      _arguments -A "-*" -C -S -s -w \
> > 
> > Oliver remarked on the -w earlier, are you quite sure it's correct?
> > 
> >               -w     In combination with -s, allow option stacking even if one
> >                      or more of the options take arguments.  For example, if
> >                      -x takes an argument, with no -s, `-xy' is considered as
> >                      a single (unhandled) option; with -s, -xy is an option
> >                      with the argument `y'; with both -s and -w, -xy may be
> >                      the option -x and the option -y with arguments still to
> >                      come.
> > 
> > (-s and -w are options to _arguments, -xy is a word on the command line
> > being completed)
> 
> I mentioned that virsh accepts all the following: -r -d 0, -r -d0, -rd
> 0, or -rd0. So to me it would seem that -s -w is correct here, right?

All these require only -s [without -w].

Does virsh accept «-dr 0» where the 0 is argument to -d [regardless of
whether -r takes an argument]?  _That_'s what -w is about.

Could you suggest how to clarify the man page section I quoted?

Cheers,

Daniel

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Marko Myllynen]

Hi,

On 2016-07-21 09:57, Daniel Shahaf wrote:
> Marko Myllynen wrote on Wed, Jul 20, 2016 at 11:36:36 +0300:
>> On 2016-07-20 09:58, Daniel Shahaf wrote:
>>> Marko Myllynen wrote on Mon, Jul 18, 2016 at 15:06:52 +0300:
>>>> +        _cache_virsh_cmds[$_cache_virsh_cmds[(i)$icmd]]=()
>>>> +      done
>>>>      fi
>>>>      if (( ! $+_cache_virsh_cmdopts )); then
>>>>        typeset -gA _cache_virsh_cmdopts
>>>>      fi
>>>>      _arguments -A "-*" -C -S -s -w \
>>>
>>> Oliver remarked on the -w earlier, are you quite sure it's correct?
>>>
>>>               -w     In combination with -s, allow option stacking even if one
>>>                      or more of the options take arguments.  For example, if
>>>                      -x takes an argument, with no -s, `-xy' is considered as
>>>                      a single (unhandled) option; with -s, -xy is an option
>>>                      with the argument `y'; with both -s and -w, -xy may be
>>>                      the option -x and the option -y with arguments still to
>>>                      come.
>>>
>>> (-s and -w are options to _arguments, -xy is a word on the command line
>>> being completed)
>>
>> I mentioned that virsh accepts all the following: -r -d 0, -r -d0, -rd
>> 0, or -rd0. So to me it would seem that -s -w is correct here, right?
> 
> All these require only -s [without -w].
> 
> Does virsh accept «-dr 0» where the 0 is argument to -d [regardless of
> whether -r takes an argument]?  _That_'s what -w is about.

Aha, no, it doesn't - yes that sounds very rare indeed, so much that I
didn't even think of that scenario.

> Could you suggest how to clarify the man page section I quoted?

Perhaps change "with both -s and -w, -xy may be the option -x and the
option -y with arguments still to come" to "... with arguments still to
come for -x"?

Thanks,

-- 
Marko Myllynen

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Oliver Kiddle]

Marko Myllynen wrote:
> +      _values servers ${=${(S)${${(f)$(sudo virt-admin srv-list)}##*--- }//[0-9]* }} && return 0

> +      _values servers ${=${${(f):-"$(sudo virt-admin srv-clients-list --server $srv)"}/ [a-z]*}//[^0-9]} && return 0

I have pushed the change but I've just noticed these two lines. I'm not
sure it is a good idea to be running sudo within completion functions.
That can trigger logging and is often denied when you're already root.
And there may be alternatives like doas.

There's also another instance of this in Mandriva/Command/_rebootin.

Anyone, have any thoughts on how this should be handled? I'm inclined
to think that users should have to specifically set a gain-root
style to enable this. Or perhaps it could only trigger if you
complete after sudo virt-admin.

Oliver

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Marko Myllynen]

Hi,

On 2016-07-21 19:12, Oliver Kiddle wrote:
> Marko Myllynen wrote:
>> +      _values servers ${=${(S)${${(f)$(sudo virt-admin srv-list)}##*--- }//[0-9]* }} && return 0
> 
>> +      _values servers ${=${${(f):-"$(sudo virt-admin srv-clients-list --server $srv)"}/ [a-z]*}//[^0-9]} && return 0
> 
> I have pushed the change but I've just noticed these two lines. I'm not
> sure it is a good idea to be running sudo within completion functions.
> That can trigger logging and is often denied when you're already root.
> And there may be alternatives like doas.
> 
> There's also another instance of this in Mandriva/Command/_rebootin.
> 
> Anyone, have any thoughts on how this should be handled? I'm inclined
> to think that users should have to specifically set a gain-root
> style to enable this. Or perhaps it could only trigger if you
> complete after sudo virt-admin.

FWIW, I doublechecked the situation off-list with the virt-admin author
and currently there are no plans to allow non-root/non-sudo users to use
virt-admin (unlike with, e.g., virsh where it is already easy to
configure to allow full access for non-root users).

Thanks,

-- 
Marko Myllynen

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Daniel Shahaf]

Marko Myllynen wrote on Thu, Jul 21, 2016 at 15:32:14 +0300:
> On 2016-07-21 09:57, Daniel Shahaf wrote:
> > Could you suggest how to clarify the man page section I quoted?
> 
> Perhaps change "with both -s and -w, -xy may be the option -x and the
> option -y with arguments still to come" to "... with arguments still to
> come for -x"?

Okay:

diff --git a/Doc/Zsh/compsys.yo b/Doc/Zsh/compsys.yo
index 8792324..a50df99 100644
--- a/Doc/Zsh/compsys.yo
+++ b/Doc/Zsh/compsys.yo
@@ -3591,8 +3591,9 @@ even if one or more of the options take
 arguments.  For example, if tt(-x) takes an argument, with no
 tt(-s), `tt(-xy)' is considered as a single (unhandled) option; with
 tt(-s), tt(-xy) is an option with the argument `tt(y)'; with both tt(-s)
-and tt(-w), tt(-xy) may be the option tt(-x) and the option tt(-y) with
-arguments still to come.
+and tt(-w), tt(-xy) is the option tt(-x) and the option tt(-y) with
+arguments to tt(-x) (and to tt(-y), if it takes arguments) still to come
+in subsequent words.
 )
 item(tt(-W))(
 This option takes tt(-w) a stage further:  it is possible to

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Daniel Shahaf]

Oliver Kiddle wrote on Thu, Jul 21, 2016 at 18:12:49 +0200:
> Marko Myllynen wrote:
> > +      _values servers ${=${(S)${${(f)$(sudo virt-admin srv-list)}##*--- }//[0-9]* }} && return 0
> 
> > +      _values servers ${=${${(f):-"$(sudo virt-admin srv-clients-list --server $srv)"}/ [a-z]*}//[^0-9]} && return 0
> 
> I have pushed the change but I've just noticed these two lines. I'm not
> sure it is a good idea to be running sudo within completion functions.
> That can trigger logging and is often denied when you're already root.
> And there may be alternatives like doas.
> 
> There's also another instance of this in Mandriva/Command/_rebootin.
> 
> Anyone, have any thoughts on how this should be handled? I'm inclined
> to think that users should have to specifically set a gain-root
> style to enable this. Or perhaps it could only trigger if you
> complete after sudo virt-admin.

In principle, I fully agree with you, with two differences:

First, in addition to 'zstyle -t … gain-root' and
'(( $+funcstack[(r)_sudo] ))' as the conditions for invoking sudo,
I think a third alternative is to use _call_program and let the user set
the 'command' style to '-sudo'.

Secondly, you don't touch on what we would do when the 'gain-root' style
is unset.  Given Marko's later email that virt-admin is not usable by
non-root users, perhaps we should do this:
.
    if (( EUID == 0 )); then
      # call 'virt-admin'
    elif The 'gain-root' or 'command' style is set; then
      # call virt-admin with sudo (or whatever the style prescribes)
    else
      _message "zsh: _libvirt: can't list completions because the 'gain-root' style is unset"
      return 1
    fi
.
?

The error message can also include the 'zstyle' incantation that sets
the style appropriately, to make it easier for the user.

Also, we should sanitize that $srv parameter before passing it to
a command run as root.  I'm not sure how virt-admin's command-line
parser works: is there any possibility for $srv to be tokenised as
anything other than the argument of the --server option?

Cheers,

Daniel

> Oliver

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Marko Myllynen]

Hi,

On 2016-07-22 09:30, Daniel Shahaf wrote:
> Marko Myllynen wrote on Thu, Jul 21, 2016 at 15:32:14 +0300:
>> On 2016-07-21 09:57, Daniel Shahaf wrote:
>>> Could you suggest how to clarify the man page section I quoted?
>>
>> Perhaps change "with both -s and -w, -xy may be the option -x and the
>> option -y with arguments still to come" to "... with arguments still to
>> come for -x"?
> 
> diff --git a/Doc/Zsh/compsys.yo b/Doc/Zsh/compsys.yo
> index 8792324..a50df99 100644
> --- a/Doc/Zsh/compsys.yo
> +++ b/Doc/Zsh/compsys.yo
> @@ -3591,8 +3591,9 @@ even if one or more of the options take
>  arguments.  For example, if tt(-x) takes an argument, with no
>  tt(-s), `tt(-xy)' is considered as a single (unhandled) option; with
>  tt(-s), tt(-xy) is an option with the argument `tt(y)'; with both tt(-s)
> -and tt(-w), tt(-xy) may be the option tt(-x) and the option tt(-y) with
> -arguments still to come.
> +and tt(-w), tt(-xy) is the option tt(-x) and the option tt(-y) with
> +arguments to tt(-x) (and to tt(-y), if it takes arguments) still to come
> +in subsequent words.
>  )
>  item(tt(-W))(
>  This option takes tt(-w) a stage further:  it is possible to

Thanks, this looks clear now.

Cheers,

-- 
Marko Myllynen

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Oliver Kiddle]

On 22 Jul, Daniel Shahaf wrote:
> In principle, I fully agree with you, with two differences:
>
> First, in addition to 'zstyle -t … gain-root' and
> '(( $+funcstack[(r)_sudo] ))' as the conditions for invoking sudo,
> I think a third alternative is to use _call_program and let the user set
> the 'command' style to '-sudo'.

It'd be best to put such logic in _call_program and _sudo so that
it doesn't get duplicated in lots of places.

> Secondly, you don't touch on what we would do when the 'gain-root' style
> is unset.  Given Marko's later email that virt-admin is not usable by
> non-root users, perhaps we should do this:

I'd suggest that it doesn't do anything special. Just run virsh
list. Testing EUID might be correct but you can't actually be sure
it won't work, perhaps due to groups or some RBAC mechanism. If it
doesn't you'd end up with a message like:
  No matches for: `domains'
In some cases, _wanted -x should perhaps be used.

Putting the logic in _call_program also has an impact on this
question because while virt-admin might be unusable to non-root
users, some other commands might be partially usable. sudo may
just allow it to get more matches.

How about something like the following? _sudo sets the _comp_priv_prefix
variable to provide a prefix to match those for the current
command-line. If _call_program is called with -p, it will default
to using this prefix unless overridden, either by a gain-privs style
or the command style with a - prefix.

This doesn't include an (( EUID )) check because it might be applicable
where permissions other than root are gained. It'd perhaps be useful if
the -p option to _call_program also caused it to add something to
$curcontext when looking up the command style but I'm not sure where
that could be done as we already have the tag and argument fields
filled. Any ideas?

Besides -u/--user, are other sudo options needed?

Oliver

diff --git a/Completion/Base/Core/_main_complete b/Completion/Base/Core/_main_complete
index 9c4cfac..c292ce7 100644
--- a/Completion/Base/Core/_main_complete
+++ b/Completion/Base/Core/_main_complete
@@ -38,6 +38,7 @@ local func funcs ret=1 tmp _compskip format nm call match min max i num\
       _saved_colors="$ZLS_COLORS" \
       _saved_colors_set=${+ZLS_COLORS} \
       _ambiguous_color=''
+local -a _comp_priv_prefix
 
 # _precommand sets this to indicate we are following a precommand modifier
 local -a precommands
diff --git a/Completion/Base/Utility/_call_program b/Completion/Base/Utility/_call_program
index 010e094..f61180a 100644
--- a/Completion/Base/Utility/_call_program
+++ b/Completion/Base/Utility/_call_program
@@ -1,6 +1,13 @@
 #autoload +X
 
 local tmp err_fd=-1
+local -a prefix
+
+if [[ "$1" = -p ]]; then
+  shift
+  zstyle -T ":completion:${curcontext}:${1}" gain-privs &&
+      prefix=( $_comp_priv_prefix )
+fi
 
 if (( ${debug_fd:--1} > 2 )) || [[ ! -t 2 ]]
 then exec {err_fd}>&2	# debug_fd is saved stderr, 2 is trace or redirect
@@ -13,10 +20,10 @@ if zstyle -s ":completion:${curcontext}:${1}" command tmp; then
   if [[ "$tmp" = -* ]]; then
     eval "$tmp[2,-1]" "$argv[2,-1]"
   else
-    eval "$tmp"
+    eval $prefix "$tmp"
   fi
 else
-  eval "$argv[2,-1]"
+  eval $prefix "$argv[2,-1]"
 fi 2>&$err_fd
 
 } always {
diff --git a/Completion/Unix/Command/_libvirt b/Completion/Unix/Command/_libvirt
index c855ac9..98c6a95 100644
--- a/Completion/Unix/Command/_libvirt
+++ b/Completion/Unix/Command/_libvirt
@@ -96,7 +96,7 @@ case $state in
     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]*}
+      _cache_virsh_cmdopts[$cmd]=${(M)${${${${=${(f)"$(_call_program options virsh help $cmd 2>&1)"}}/\[}/\]}/\;}:#-[-0-9A-Za-z]*}
     fi
     _values -w options ${=_cache_virsh_cmdopts[$cmd]} && ret=0
   ;;
@@ -110,7 +110,7 @@ case $state in
     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)$(_call_program -p servers virt-admin srv-list)}##*--- }//[0-9]* }} && return 0
     fi
     if [[ $words[-2] == --client ]]; then
       local srv
@@ -118,10 +118,10 @@ case $state in
         [[ $words[$i] == --server ]] && srv=$words[$i+1] && break
       done
       [[ -z $srv ]] && return 1
-      _values servers ${=${${(f):-"$(sudo virt-admin srv-clients-list --server $srv)"}/ [a-z]*}//[^0-9]} && return 0
+      _values servers ${=${${(f):-"$(_call_program -p servers 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]*}
+      _cache_virt_admin_cmdopts[$cmd]=${(M)${${${${=${(f)"$(_call_program options virt-admin help $cmd 2>&1)"}}/\[}/\]}/\;}:#-[-0-9A-Za-z]*}
     fi
     [[ -n $_cache_virt_admin_cmdopts[$cmd] ]] && \
       _values -w options ${=_cache_virt_admin_cmdopts[$cmd]} && ret=0
diff --git a/Completion/Unix/Command/_sudo b/Completion/Unix/Command/_sudo
index 63ac37f..1ceefa2 100644
--- a/Completion/Unix/Command/_sudo
+++ b/Completion/Unix/Command/_sudo
@@ -48,7 +48,7 @@ else
     '(-H --set-home -i --login -s --shell -e --edit)'{-H,--set-home}"[set HOME variable to target user's home dir]" \
     '(-P --preserve-groups -i -login -s --shell -e --edit)'{-P,--preserve-groups}"[preserve group vector instead of setting to target's]" \
     '(-)1:command: _command_names -e'
-    '*::arguments: _normal'
+    '*::arguments:{ _comp_priv_prefix=( $words[1] -n ${(kv)opt_args[(I)(-u|--user)]} ) ; _normal }'
   )
 fi
 

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Daniel Shahaf]

Oliver Kiddle wrote on Wed, Aug 31, 2016 at 23:15:20 +0200:
> On 22 Jul, Daniel Shahaf wrote:
> > Secondly, you don't touch on what we would do when the 'gain-root' style
> > is unset.  Given Marko's later email that virt-admin is not usable by
> > non-root users, perhaps we should do this:
> 
> I'd suggest that it doesn't do anything special. Just run virsh
> list. Testing EUID might be correct but you can't actually be sure
> it won't work, perhaps due to groups or some RBAC mechanism. If it
> doesn't you'd end up with a message like:
>   No matches for: `domains'
> In some cases, _wanted -x should perhaps be used.
> 
> Putting the logic in _call_program also has an impact on this
> question because while virt-admin might be unusable to non-root
> users, some other commands might be partially usable. sudo may
> just allow it to get more matches.

All this makes sense.

However, your patch seems to implement something slightly different: the
patch executes sudo when (-p was passed, and) 'gain-privs' is either
unset or set to true, whereas the above paragraphs seem to be arguing
for *not* using sudo when the style is unset?  Perhaps I misunderstood
your argument.

I have no strong opinion regarding what gain-privs should default to
when unspecified.  

> How about something like the following? _sudo sets the _comp_priv_prefix
> variable to provide a prefix to match those for the current
> command-line. If _call_program is called with -p, it will default
> to using this prefix unless overridden, either by a gain-privs style
> or the command style with a - prefix.
> 

This approach also makes sense to me.  It does, however, require us
(completion file authors) to be *very* careful / disciplined with uses
of -p to avoid possibly executing parts of the command line as root.

Executing a command as sudo upon pressing <TAB> could have undesired
results in two ways: if the user _has_ sudo, executing the command with
sudo could surprise her; and if user _doesn't_ have sudo, executing the
command might result in an email from sudo to the root user.

This change might deserve a NEWS entry?

> This doesn't include an (( EUID )) check because it might be applicable
> where permissions other than root are gained.

I see: the privileges gained need not be privileges on the local system.

>
> It'd perhaps be useful if
> the -p option to _call_program also caused it to add something to
> $curcontext when looking up the command style but I'm not sure where
> that could be done as we already have the tag and argument fields
> filled. Any ideas?

I suppose it fits best in the "command" part of the context, e.g.,
.
    _f() { : $(_call_program lipsum echo lorem ipsum) }
    sudo f <TAB>
.
could look up :completion::complete:f-under-sudo::lipsum or so.
(Replace "f-under-sudo" by any string that identifies both 'f' and
'sudo' unambiguously.)

I don't think there's a compatibility problem here: if we add -p to an
existing _call_program invocation we should change the tag that
invocation uses, so might at the same time change the context the tag is
looked up in.

> Besides -u/--user, are other sudo options needed?
> 

All of these seem potentially needed:

-g -P    (whom to execute as)
-r -t    (on my system they're about SELinux; not sure about their meaning on other platforms)
-h       (where to execute)
-H -E -i (execution environment)

That's based on the sudo manpage in Debian stable, sudo 1.8.10.

Come to think of it, sudoers(5) allows per-command defaults, so if the
completion of the command 'foo' invokes «_call_program -b bar», the
sudoers(5) Defaults!foo clauses wouldn't fire.  Hopefully that will do
nothing worse than failing to find completion.

> Oliver
> 
> diff --git a/Completion/Base/Core/_main_complete b/Completion/Base/Core/_main_complete
> index 9c4cfac..c292ce7 100644
> --- a/Completion/Base/Core/_main_complete
> +++ b/Completion/Base/Core/_main_complete
> @@ -38,6 +38,7 @@ local func funcs ret=1 tmp _compskip format nm call match min max i num\
>        _saved_colors="$ZLS_COLORS" \
>        _saved_colors_set=${+ZLS_COLORS} \
>        _ambiguous_color=''
> +local -a _comp_priv_prefix

Could we please not introduce any more short opaque names?  "priv" is
ambiguous/unclear.  Perhaps _comp_gain_privs_prefix.  (Or perhaps the
style name should be "gain-privileges".)

> diff --git a/Completion/Unix/Command/_sudo b/Completion/Unix/Command/_sudo
> index 63ac37f..1ceefa2 100644
> --- a/Completion/Unix/Command/_sudo
> +++ b/Completion/Unix/Command/_sudo
> @@ -48,7 +48,7 @@ else
>      '(-H --set-home -i --login -s --shell -e --edit)'{-H,--set-home}"[set HOME variable to target user's home dir]" \
>      '(-P --preserve-groups -i -login -s --shell -e --edit)'{-P,--preserve-groups}"[preserve group vector instead of setting to target's]" \
>      '(-)1:command: _command_names -e'
> -    '*::arguments: _normal'
> +    '*::arguments:{ _comp_priv_prefix=( $words[1] -n ${(kv)opt_args[(I)(-u|--user)]} ) ; _normal }'

You removed the space that followed the last colon; wouldn't that cause
_arguments to try and exec the anonymous block with arguments?  I.e.,
"*::arguments:foo" executes «foo $expl», so wouldn't the above line
cause «{ ... ; _normal } $expl» to be executed?

Should $_comp_priv_prefix be left unset when (( ${+funcstack[(r)_ssh]} ))?

Thanks for taking care of this.

Cheers,

Daniel

>    )
>  fi
>  
> 

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Oliver Kiddle]

Daniel Shahaf wrote:
> However, your patch seems to implement something slightly different: the
> patch executes sudo when (-p was passed, and) 'gain-privs' is either
> unset or set to true, whereas the above paragraphs seem to be arguing
> for *not* using sudo when the style is unset?  Perhaps I misunderstood
> your argument.

The code is what I intended.

It also requires sudo to be present on the command-line being typed
so it seemed reasonable. And it is consistent with the remote-access
style.  I don't have a strong opinion either, however. It was the
unconditionally hard-coded sudo that I did have a strong opinion
on.

> Executing a command as sudo upon pressing <TAB> could have undesired
> results in two ways: if the user _has_ sudo, executing the command with
> sudo could surprise her; and if user _doesn't_ have sudo, executing the
> command might result in an email from sudo to the root user.

If a user does:
  sudo virsh start --domain <tab>
then yes, it might send off a mail to root but they'd get that anyway
when they actually try to run the command they're composing.

> I see: the privileges gained need not be privileges on the local system.

Or it might be something else like switch to the httpd user to run the
command.

> > It'd perhaps be useful if
> > the -p option to _call_program also caused it to add something to
> > $curcontext when looking up the command style but I'm not sure where
> > that could be done as we already have the tag and argument fields
> > filled. Any ideas?
>
> I suppose it fits best in the "command" part of the context, e.g.,

> could look up :completion::complete:f-under-sudo::lipsum or so.

Perhaps f/sudo? / can't appear in the command name.

> Come to think of it, sudoers(5) allows per-command defaults, so if the
> completion of the command 'foo' invokes «_call_program -b bar», the
> sudoers(5) Defaults!foo clauses wouldn't fire.  Hopefully that will do
> nothing worse than failing to find completion.

At worst, the command will be blocked with a root e-mail. User will need
to set the command or gain-privileges style.

I also wonder if the prefix perhaps has other uses. With env for
instance.

> Could we please not introduce any more short opaque names?  "priv" is
> ambiguous/unclear.  Perhaps _comp_gain_privs_prefix.  (Or perhaps the
> style name should be "gain-privileges".)

_comp_gain_privs_prefix starts getting a bit long. And is less
user-visible. I'm happy with gain-privileges on the style.

> > +    '*::arguments:{ _comp_priv_prefix=( $words[1] -n ${(kv)opt_args[(I)(-u|--user)]} )
 ; _normal }'
> You removed the space that followed the last colon; wouldn't that cause

The { … } form is a specifically handled form by _arguments and it
won't insert any arguments. So I think this should be fine.

The patch below is just documentation.

I felt that _comp_priv_prefix itself ought to be documented but
there wasn't an obvious place to put it. I could try to squeeze it
in under gain-privileges but is there any thoughts on adding a
section on standard variables as follows? Zsh convention would
probably be to use "parameters" instead of "variables" and I'll do
that if anyone objects but we might be doing our user's a favour
by ditching that particular convention.

I also inserted the otherwise undocumented _comp_caller_options
into that new section. Any thoughts on what else might go there?
curcontext, _compskip and service are covered elsewhere but it might
be clearer to reorganise them into this section.

I also moved the comppostfuncs documentation to this section. Does
that seem reasonable?

> Should $_comp_priv_prefix be left unset when (( ${+funcstack[(r)_ssh]} ))?

Yes. Very good point. And as a note to myself, I shouldn't have
sudo put any command substitutions into _comp_priv_prefix.

Oliver

diff --git a/Doc/Zsh/compsys.yo b/Doc/Zsh/compsys.yo
index 9e49913..dab73ef 100644
--- a/Doc/Zsh/compsys.yo
+++ b/Doc/Zsh/compsys.yo
@@ -1815,6 +1815,14 @@ In the case of the tt(_match) completer, the style may also be set to
 the string `tt(pattern)'.  Then the pattern on the line is left
 unchanged if it does not match unambiguously.
 )
+kindex(gain-privileges, completion style)
+item(tt(gain-privileges))(
+If set to tt(false), certain commands will be prevented from making
+use of commands like tt(sudo) or tt(doas) to gain extra privileges when
+retrieving information for completion. By default, this would only be
+done when a command such as tt(sudo) appears on the command-line. To
+force the use of, e.g. tt(sudo), the tt(command) style can be used.
+)
 kindex(keep-prefix, completion style)
 item(tt(keep-prefix))(
 This style is used by the tt(_expand) completer.  If it is `true', the
@@ -3400,12 +3408,6 @@ generating matches all follow the convention of returning status zero if they
 generated completions and non-zero if no matching completions could be 
 added.
 
-Two more features are offered by the tt(_main_complete) function.  The
-arrays tt(compprefuncs) and tt(comppostfuncs) may contain
-names of functions that are to be called immediately before or after
-completion has been tried.  A function will only be called once unless
-it explicitly reinserts itself into the array.
-
 startitem()
 findex(_all_labels)
 item(tt(_all_labels) [ tt(-x) ] [ tt(-12VJ) ] var(tag) var(name) var(descr) [ var(command) var(args) ... ])(
@@ -4012,7 +4014,7 @@ The return status of tt(_call_function) itself is zero if the function
 var(name) exists and was called and non-zero otherwise.
 )
 findex(_call_program)
-item(tt(_call_program) var(tag) var(string) ...)(
+item(tt(_call_program) [ tt(-p) ] var(tag) var(string) ...)(
 This function provides a mechanism for the user to override the use of an
 external command.  It looks up the tt(command) style with the supplied
 var(tag).  If the style is set, its value is used as the command to
@@ -4020,6 +4022,13 @@ execute.  The var(string)s from the call to tt(_call_program), or from the
 style if set, are concatenated with spaces between them and the resulting
 string is evaluated.  The return status is the return status of the command
 called.
+
+If the option `tt(-p)' is supplied it indicates that the command output
+is influenced by the permissions it is run with. Unless disabled with
+the tt(gain-privileges) style or overidden with the tt(command) style,
+tt(_call_program) will make use of commands such as tt(sudo) if
+present on the command-line, to match the permissions to whatever the
+final command is likely to run under.
 )
 findex(_combination)
 item(tt(_combination) [ tt(-s) var(pattern) ] var(tag) var(style) var(spec) ... var(field) var(opts) ...)(
@@ -4875,7 +4884,38 @@ the same meaning as for tt(_description).
 )
 enditem()
 
-texinode(Completion Directories)()(Completion Functions)(Completion System)
+texinode(Completion System Variables)(Completion Directories)(Completion Functions)(Completion System)
+sect(Completion System Variables)
+cindex(completion system, variables)
+
+There are some standard variables, initialised by the tt(_main_complete)
+function and then used from other functions.
+
+The standard variables are:
+
+startitem()
+item(tt(_comp_caller_options))(
+The completion system uses tt(setopt) to set a number of options. This
+allows functions to be written without concern for compatibility with
+every possible combination of user options. However, sometimes completion
+needs to know what the user's option preferences are. These are saved
+in the tt(_comp_caller_options) associative array.
+)
+
+item(tt(_comp_priv_prefix))(
+Completion functions such as tt(_sudo) can set the tt(_comp_priv_prefix)
+array to a command that may then be used by tt(_call_program) to
+match the permissions when calling programs to generate matches.
+)
+enditem()
+
+Two more features are offered by the tt(_main_complete) function.  The
+arrays tt(compprefuncs) and tt(comppostfuncs) may contain
+names of functions that are to be called immediately before or after
+completion has been tried.  A function will only be called once unless
+it explicitly reinserts itself into the array.
+
+texinode(Completion Directories)()(Completion System Variables)(Completion System)
 sect(Completion Directories)
 cindex(completion system, directory structure)
 

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Daniel Shahaf]

Oliver Kiddle wrote on Fri, Sep 02, 2016 at 17:02:09 +0200:
> Daniel Shahaf wrote:
> > However, your patch seems to implement something slightly different: the
> > patch executes sudo when (-p was passed, and) 'gain-privs' is either
> > unset or set to true, whereas the above paragraphs seem to be arguing
> > for *not* using sudo when the style is unset?  Perhaps I misunderstood
> > your argument.
> 
> The code is what I intended.
> 
> It also requires sudo to be present on the command-line being typed
> so it seemed reasonable. And it is consistent with the remote-access
> style.  I don't have a strong opinion either, however. It was the
> unconditionally hard-coded sudo that I did have a strong opinion
> on.
> 
> > Executing a command as sudo upon pressing <TAB> could have undesired
> > results in two ways: if the user _has_ sudo, executing the command with
> > sudo could surprise her; and if user _doesn't_ have sudo, executing the
> > command might result in an email from sudo to the root user.
> 
> If a user does:
>   sudo virsh start --domain <tab>
> then yes, it might send off a mail to root but they'd get that anyway
> when they actually try to run the command they're composing.
> 

Well, we shouldn't spam the root user.  In particular, I'm concerned
about several completion attempts in quick succession (pretty common,
both when typing in a command and when using completion as a man page
/ reference) causing several sudo emails in quick succession.  Sysadmins
tend to be jumpy about this sort of thing.

Perhaps using sudo should be opt-in on the part of the user; that is:
when gain-privileges is unset, either behave as though it's set to
false, or perhaps prompt the user (via the minibuffer?) for permission
to use sudo.  (And cache the user's reply for next time?)

Just thinking out loud here.

> > > This doesn't include an (( EUID )) check because it might be applicable
> > > where permissions other than root are gained.
> > 
> > I see: the privileges gained need not be privileges on the local system.
> 
> Or it might be something else like switch to the httpd user to run the
> command.
> 

It might be, yes, but how is this an example of why testing EUID is not
appropriate?  If a completion function needs to run a command as httpd,
then it has every reason to test EUID:
.
    case $EUID in
    (`id -u httpd`) _call_program $desc $args;;
    (`id -u  root`) (UID=`id -u httpd` && _call_program $desc $args);;
                (*) _call_program -p $desc $args;;
    esac

By the way: assigning to UID doesn't set $? :
.
    % UID=42; echo $?
    zsh: failed to change user ID: operation not permitted
    0

> > > It'd perhaps be useful if
> > > the -p option to _call_program also caused it to add something to
> > > $curcontext when looking up the command style but I'm not sure where
> > > that could be done as we already have the tag and argument fields
> > > filled. Any ideas?
> >
> > I suppose it fits best in the "command" part of the context, e.g.,
> 
> > could look up :completion::complete:f-under-sudo::lipsum or so.
> 
> Perhaps f/sudo? / can't appear in the command name.
> 

+1, sounds good.

How do we handle the case that >1 precommand wants to set _comp_priv_prefix?
For example, suppose ssh learnt to set $_comp_priv_prefix (I'm not
proposing to teach it to do so; this is just for the sake of example),
and somebody completed
.
    ssh -t $host sudo virt-admin <TAB>
.
, would _comp_priv_prefix then be set to (ssh -t $host sudo) and would the
style context be f/sudo/ssh?

> > Come to think of it, sudoers(5) allows per-command defaults, so if the
> > completion of the command 'foo' invokes «_call_program -b bar», the
> > sudoers(5) Defaults!foo clauses wouldn't fire.  Hopefully that will do
> > nothing worse than failing to find completion.
> 
> At worst, the command will be blocked with a root e-mail. User will need
> to set the command or gain-privileges style.
> 
> I also wonder if the prefix perhaps has other uses. With env for
> instance.
> 

You can set the 'command' style to an anonymous function; I'm sure
people have found creative uses for this.

E.g., poor man's profiling:
% zstyle \* command '-() { typeset -F SECONDS; { "$@" } always { print -ru 10 -- "$SECONDS" } }'

Perhaps it's possible to use a prefix to arrange for the command to run
asynchronously.

> > > +    '*::arguments:{ _comp_priv_prefix=( $words[1] -n ${(kv)opt_args[(I)(-u|--user)]} )
>  ; _normal }'
> > You removed the space that followed the last colon; wouldn't that cause
> 
> The { … } form is a specifically handled form by _arguments and it
> won't insert any arguments. So I think this should be fine.
> 

My bad — I missed the fact that the {} form of a spec's action is
documented because I had misgrepped the man page.

> I felt that _comp_priv_prefix itself ought to be documented but
> there wasn't an obvious place to put it. I could try to squeeze it
> in under gain-privileges but is there any thoughts on adding a
> section on standard variables as follows?
>

I don't have a strong opinion on this question.

> Zsh convention would probably be to use "parameters" instead of
> "variables" and I'll do that if anyone objects but we might be doing
> our user's a favour by ditching that particular convention.

+1 to "variables".

> +++ b/Doc/Zsh/compsys.yo
> @@ -1815,6 +1815,14 @@ In the case of the tt(_match) completer, the style may also be set to
> @@ -4020,6 +4022,13 @@ execute.  The var(string)s from the call to tt(_call_program), or from the
> +If the option `tt(-p)' is supplied it indicates that the command output
> +is influenced by the permissions it is run with. Unless disabled with
> +the tt(gain-privileges) style or overidden with the tt(command) style,

Typo: "overridden"

Also, 'command' only overrides 'gain-privileges' if the former's value
starts with a hyphen.

> +tt(_call_program) will make use of commands such as tt(sudo) if
> +present on the command-line, to match the permissions to whatever the
> +final command is likely to run under.

Style: suggest to strike "to whatever".

> @@ -4875,7 +4884,38 @@ the same meaning as for tt(_description).
> +item(tt(_comp_caller_options))(
> +The completion system uses tt(setopt) to set a number of options. This
> +allows functions to be written without concern for compatibility with
> +every possible combination of user options. However, sometimes completion
> +needs to know what the user's option preferences are. These are saved
> +in the tt(_comp_caller_options) associative array.

What are the keys and values of the array?  Suggested text:

    The array maps option names, spelled in lowercase without
    underscores, to the string `tt(on)' or `tt(off)'.

It wouldn't be correct to say 'the keys are the same as those of
$options' because keys with underscores or mixed case work in $options
but not int $_comp_caller_options.

> +item(tt(_comp_priv_prefix))(
> +Completion functions such as tt(_sudo) can set the tt(_comp_priv_prefix)
> +array to a command that may then be used by tt(_call_program) to
> +match the permissions when calling programs to generate matches.

Suggest to change "command" to "prefix command" (assuming that's
a well-known term for "a command that does execve(argv+1)", such as env,
nohup, etc.) to make it clear to authors of functions such as _doas what
they should set $_comp_priv_prefix to.

Also, see above about f/ssh/sudo; if we accept that use-case then the
docs here might say the array should be appended to, not clobbered.

> 

While we're about _call_program, it inspects the variable $debug_fd.
That variable is set by _complete_debug but nothing in the normal
completion codepath unsets it, so if the user happens to have a variable
by that name, then _call_program would use it.

I assume $debug_fd needs to be either documented or unset in
_main_complete or renamed as _comp_*.

Cheers,

Daniel

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Daniel Shahaf]

Oliver Kiddle wrote on Fri, Sep 02, 2016 at 17:02:09 +0200:
> I also inserted the otherwise undocumented _comp_caller_options
> into that new section. Any thoughts on what else might go there?

There's a $redirections parameter in comprparams, added in 16751
(10490ec4).  It is unused and appears to never have been documented, so
perhaps we should just remove it.

% git log -S '[(]redirections[)]' Doc/Zsh
% 

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Bart Schaefer]

On Sep 4,  4:01am, Daniel Shahaf wrote:
}
} Perhaps using sudo should be opt-in on the part of the user; that is:
} when gain-privileges is unset, either behave as though it's set to
} false, or perhaps prompt the user (via the minibuffer?) for permission
} to use sudo.  (And cache the user's reply for next time?)
} 
} Just thinking out loud here.

I haven't been paying a whole lot of attention to this thread because
Oliver seemed to have it pretty well surrounded, but my two cents:

I would prefer that we never implicitly call 'sudo'; rather, that it
be explictly referenced by the user in a zstyle whether or not it's
already on the command line.

There's an admittedly somewhat arbitrary line between a completion
being helpful enough, and it venturing into places it has no business
going in an effort to present perfect knowledge.  Sometimes perfect
knowledge isn't worth the effort or the side-effects.  (A lot of the
"remote" completions are bumping along this line as well.)

However, I also think prompting during completion (except in the form
of the menu-ing variations) is likely to cause mistakes.

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Oliver Kiddle]

On 6 Sep, Bart wrote:
> I would prefer that we never implicitly call 'sudo'; rather, that it
> be explictly referenced by the user in a zstyle whether or not it's
> already on the command line.

Attached is an updated patch. With this you now have to explicitly set
the gain-privileges and use sudo on the command-line before it will ever
use sudo.

This also includes the documentation changes again but with some
alterations. I'll separately look at expanding the new variables
section of the documentation. $redirections is applicable to compwid,
not compsys, however.

I checked over functions that use _normal or _cmdstring. _pfexec
is the only other function that now sets _comp_priv_prefix. It is
emptied in quite a few: dsh, fsh, rlogin, ssh and mosh are for
remote commands, and jexec, dchroot*, schroot, zlogin run a command
in a container or chroot. I'm a bit unsure about whether it should also
apply to vserver.

The patch also now changes the Mandriva rebootin function which had
a hard-coded sudo.

Oliver

diff --git a/Completion/BSD/Command/_jexec b/Completion/BSD/Command/_jexec
index 279812b..85829d1 100644
--- a/Completion/BSD/Command/_jexec
+++ b/Completion/BSD/Command/_jexec
@@ -2,6 +2,7 @@
 
 _jexec_normal() {
   local PATH=$PATH
+  local -a _comp_priv_prefix
   # relative paths are relative to the jail's root
   path=( "$(command jls -j $words[1] path)"/$^path )
   shift 1 words; (( CURRENT-- ))
diff --git a/Completion/Base/Core/_main_complete b/Completion/Base/Core/_main_complete
index 9c4cfac..c292ce7 100644
--- a/Completion/Base/Core/_main_complete
+++ b/Completion/Base/Core/_main_complete
@@ -38,6 +38,7 @@ local func funcs ret=1 tmp _compskip format nm call match min max i num\
       _saved_colors="$ZLS_COLORS" \
       _saved_colors_set=${+ZLS_COLORS} \
       _ambiguous_color=''
+local -a _comp_priv_prefix
 
 # _precommand sets this to indicate we are following a precommand modifier
 local -a precommands
diff --git a/Completion/Base/Utility/_call_program b/Completion/Base/Utility/_call_program
index 010e094..95c761e 100644
--- a/Completion/Base/Utility/_call_program
+++ b/Completion/Base/Utility/_call_program
@@ -1,6 +1,13 @@
 #autoload +X
 
 local tmp err_fd=-1
+local -a prefix
+
+if [[ "$1" = -p ]]; then
+  shift
+  zstyle -t ":completion:${curcontext}:${1}" gain-privileges &&
+      prefix=( $_comp_priv_prefix )
+fi
 
 if (( ${debug_fd:--1} > 2 )) || [[ ! -t 2 ]]
 then exec {err_fd}>&2	# debug_fd is saved stderr, 2 is trace or redirect
@@ -13,10 +20,10 @@ if zstyle -s ":completion:${curcontext}:${1}" command tmp; then
   if [[ "$tmp" = -* ]]; then
     eval "$tmp[2,-1]" "$argv[2,-1]"
   else
-    eval "$tmp"
+    eval $prefix "$tmp"
   fi
 else
-  eval "$argv[2,-1]"
+  eval $prefix "$argv[2,-1]"
 fi 2>&$err_fd
 
 } always {
diff --git a/Completion/Debian/Command/_dchroot b/Completion/Debian/Command/_dchroot
index c26e569..2a6f5d8 100644
--- a/Completion/Debian/Command/_dchroot
+++ b/Completion/Debian/Command/_dchroot
@@ -2,6 +2,7 @@
 
 local expl context state line
 typeset -A opt_args
+local -a _comp_priv_prefix
 
 _arguments -S \
        '(-h --help)'{-h,--help}'[help]' \
diff --git a/Completion/Debian/Command/_dchroot-dsa b/Completion/Debian/Command/_dchroot-dsa
index d4668b5..e8e981b 100644
--- a/Completion/Debian/Command/_dchroot-dsa
+++ b/Completion/Debian/Command/_dchroot-dsa
@@ -2,6 +2,7 @@
 
 local expl context state line
 typeset -A opt_args
+local -a _comp_priv_prefix
 
 _arguments -S \
        '(-h --help)'{-h,--help}'[help]' \
diff --git a/Completion/Debian/Command/_schroot b/Completion/Debian/Command/_schroot
index 06117be..117df45 100644
--- a/Completion/Debian/Command/_schroot
+++ b/Completion/Debian/Command/_schroot
@@ -2,6 +2,7 @@
 
 local expl context state line
 typeset -A opt_args
+local -a _comp_priv_prefix
 
 _arguments -S \
        '(-h --help)'{-h,--help}'[help]' \
diff --git a/Completion/Mandriva/Command/_rebootin b/Completion/Mandriva/Command/_rebootin
index 3f30b25..284ff08 100644
--- a/Completion/Mandriva/Command/_rebootin
+++ b/Completion/Mandriva/Command/_rebootin
@@ -2,7 +2,7 @@
 
 local context state line expl
 typeset -A opt_args
-local loader=$(sudo detectloader -q)
+local loader=${$(_call_program -p entries detectloader -q):-GRUB}
 
 _arguments -s \
     '-n[no immediate reboot just set the flags for next reboot]' \
diff --git a/Completion/Solaris/Command/_pfexec b/Completion/Solaris/Command/_pfexec
index 2273362..3f1f3e7 100644
--- a/Completion/Solaris/Command/_pfexec
+++ b/Completion/Solaris/Command/_pfexec
@@ -25,7 +25,7 @@ _pfexec() {
  	_arguments \
 		'-P[privileges to acquire]:privspec:_privset' \
  		'(-):command name: _command_names -e' \
- 		'*::arguments: _normal'
+		'*::arguments:{ _comp_priv_prefix=( $words[1] ${(kv)opt_args[-P]} ) ; _normal }'
 }
 
 _pfexec "$@"
diff --git a/Completion/Solaris/Command/_zlogin b/Completion/Solaris/Command/_zlogin
index 04018eb..065f55b 100644
--- a/Completion/Solaris/Command/_zlogin
+++ b/Completion/Solaris/Command/_zlogin
@@ -1,6 +1,8 @@
 #compdef zlogin
 # Synced with the Nevada build 162 man page
 
+local -a _comp_priv_prefix
+
 _zlogin() {
 	_arguments -s \
 		'-E[Disable escape character]' \
diff --git a/Completion/Unix/Command/_dsh b/Completion/Unix/Command/_dsh
index 688e024..8c5c232 100644
--- a/Completion/Unix/Command/_dsh
+++ b/Completion/Unix/Command/_dsh
@@ -2,6 +2,7 @@
 
 local curcontext="$curcontext" state line expl
 typeset -A opt_args
+local -a _comp_priv_prefix
 
 _arguments -s -C -S \
   '(-v --verbose -q --quiet)'{-v,--verbose}'[verbose output]' \
diff --git a/Completion/Unix/Command/_fsh b/Completion/Unix/Command/_fsh
index d9ced5f..c393731 100644
--- a/Completion/Unix/Command/_fsh
+++ b/Completion/Unix/Command/_fsh
@@ -1,6 +1,7 @@
 #compdef fsh
 
 local curcontext="$curcontext" state line ret=1
+local -a _comp_priv_prefix
 
 _arguments -C \
   '(- : *)'{-h,--help}'[display help information]' \
diff --git a/Completion/Unix/Command/_libvirt b/Completion/Unix/Command/_libvirt
index 658e197..17b02be 100644
--- a/Completion/Unix/Command/_libvirt
+++ b/Completion/Unix/Command/_libvirt
@@ -155,7 +155,7 @@ case $state in
         return 1
       ;;
       --device)
-        values; values=( $(_call_program nodedevs "virsh $conn_opt nodedev-list") )
+        values; values=( $(_call_program devices "virsh $conn_opt nodedev-list") )
         [[ -n $values ]] && _wanted devices expl device compadd ${=values} && return 0
         return 1
       ;;
@@ -204,7 +204,7 @@ case $state in
       fi
     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]*}
+      _cache_virsh_cmd_opts[$cmd]=${(M)${${${${=${(f)"$(_call_program options virsh help $cmd 2>&1)"}}/\[}/\]}/\;}:#-[-0-9A-Za-z]*}
     [[ -n ${=_cache_virsh_cmd_opts[$cmd]} ]] && \
       _values -w option ${(u)=_cache_virsh_cmd_opts[$cmd]} && ret=0
   ;;
@@ -218,16 +218,16 @@ case $state in
     done
     [[ -z $cmd ]] && return 1
     if [[ $words[CURRENT-1] == --server ]]; then
-      _wanted servers expl server compadd ${=${(S)${${(f)$(sudo virt-admin ${(Q)conn_opt} srv-list)}##*--- }//[0-9]* }} && return 0
+      _wanted servers expl server compadd ${=${(S)${${(f)$(_call_program -p servers virt-admin ${(Q)conn_opt} srv-list)}##*--- }//[0-9]* }} && return 0
     fi
     if [[ $words[CURRENT-1] == --client ]]; then
       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):-"$(_call_program -p clients 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]*}
+      _cache_virt_admin_cmd_opts[$cmd]=${(M)${${${${=${(f)"$(_call_program options virt-admin help $cmd 2>&1)"}}/\[}/\]}/\;}:#-[-0-9A-Za-z]*}
     [[ -n $_cache_virt_admin_cmd_opts[$cmd] ]] && \
       _values -w option ${(u)=_cache_virt_admin_cmd_opts[$cmd]} && ret=0
   ;;
diff --git a/Completion/Unix/Command/_mosh b/Completion/Unix/Command/_mosh
index c19f6eb..431fdbf 100644
--- a/Completion/Unix/Command/_mosh
+++ b/Completion/Unix/Command/_mosh
@@ -1,6 +1,7 @@
 #compdef mosh
 
 local curcontext="$curcontext" state line
+local -a _comp_priv_prefix
 
 _arguments -C \
   '(-)--help[display help information]' \
diff --git a/Completion/Unix/Command/_rlogin b/Completion/Unix/Command/_rlogin
index a04c6d0..8f74939 100644
--- a/Completion/Unix/Command/_rlogin
+++ b/Completion/Unix/Command/_rlogin
@@ -12,6 +12,7 @@ _rlogin () {
   rsh|remsh)
     local context state line ret=1
     typeset -A opt_args
+    local -a _comp_priv_prefix
 
     _arguments -s \
       '-n[ignore stdin]' \
diff --git a/Completion/Unix/Command/_ssh b/Completion/Unix/Command/_ssh
index 7b2cdd8..5ee4fd2 100644
--- a/Completion/Unix/Command/_ssh
+++ b/Completion/Unix/Command/_ssh
@@ -605,6 +605,7 @@ _ssh () {
           hmac-sha2-256-96 hmac-sha2-512 hmac-sha2-512-96 && ret=0
       ;;
     command)
+      local -a _comp_priv_prefix
       shift 1 words
       (( CURRENT-- ))
       _normal
diff --git a/Completion/Unix/Command/_sudo b/Completion/Unix/Command/_sudo
index 63ac37f..21b1ef4 100644
--- a/Completion/Unix/Command/_sudo
+++ b/Completion/Unix/Command/_sudo
@@ -48,7 +48,7 @@ else
     '(-H --set-home -i --login -s --shell -e --edit)'{-H,--set-home}"[set HOME variable to target user's home dir]" \
     '(-P --preserve-groups -i -login -s --shell -e --edit)'{-P,--preserve-groups}"[preserve group vector instead of setting to target's]" \
     '(-)1:command: _command_names -e'
-    '*::arguments: _normal'
+    '*::arguments:{ _comp_priv_prefix=( $words[1] -n ${(kv)opt_args[(I)(-[ugHEP]|--(user|group|set-home|preserve-env|preserve-groups))]} ) ; _normal }'
   )
 fi
 
diff --git a/Doc/Zsh/compsys.yo b/Doc/Zsh/compsys.yo
index ecf17e7..0db01e9 100644
--- a/Doc/Zsh/compsys.yo
+++ b/Doc/Zsh/compsys.yo
@@ -1842,6 +1842,14 @@ In the case of the tt(_match) completer, the style may also be set to
 the string `tt(pattern)'.  Then the pattern on the line is left
 unchanged if it does not match unambiguously.
 )
+kindex(gain-privileges, completion style)
+item(tt(gain-privileges))(
+If set to tt(true), this style enables the use of commands like tt(sudo)
+or tt(doas) to gain extra privileges when retrieving information for
+completion. This is only done when a command such as tt(sudo) appears on
+the command-line. To force the use of, e.g. tt(sudo), the tt(command)
+style can be used.
+)
 kindex(keep-prefix, completion style)
 item(tt(keep-prefix))(
 This style is used by the tt(_expand) completer.  If it is `true', the
@@ -3471,12 +3479,6 @@ generating matches all follow the convention of returning status zero if they
 generated completions and non-zero if no matching completions could be 
 added.
 
-Two more features are offered by the tt(_main_complete) function.  The
-arrays tt(compprefuncs) and tt(comppostfuncs) may contain
-names of functions that are to be called immediately before or after
-completion has been tried.  A function will only be called once unless
-it explicitly reinserts itself into the array.
-
 startitem()
 findex(_absolute_command_paths)
 item(tt(_absolute_command_paths))(
@@ -4173,7 +4175,7 @@ The return status of tt(_call_function) itself is zero if the function
 var(name) exists and was called and non-zero otherwise.
 )
 findex(_call_program)
-item(tt(_call_program) var(tag) var(string) ...)(
+item(tt(_call_program) [ tt(-p) ] var(tag) var(string) ...)(
 This function provides a mechanism for the user to override the use of an
 external command.  It looks up the tt(command) style with the supplied
 var(tag).  If the style is set, its value is used as the command to
@@ -4181,6 +4183,13 @@ execute.  The var(string)s from the call to tt(_call_program), or from the
 style if set, are concatenated with spaces between them and the resulting
 string is evaluated.  The return status is the return status of the command
 called.
+
+If the option `tt(-p)' is supplied it indicates that the command
+output is influenced by the permissions it is run with. If the
+tt(gain-privileges) style is set to true, tt(_call_program) will make
+use of commands such as tt(sudo), if present on the command-line, to
+match the permissions to whatever the final command is likely to run
+under.
 )
 findex(_combination)
 item(tt(_combination) [ tt(-s) var(pattern) ] var(tag) var(style) var(spec) ... var(field) var(opts) ...)(
@@ -5073,7 +5082,38 @@ ifnzman(noderef(The zsh/zleparameter Module)).
 )
 enditem()
 
-texinode(Completion Directories)()(Completion Functions)(Completion System)
+texinode(Completion System Variables)(Completion Directories)(Completion Functions)(Completion System)
+sect(Completion System Variables)
+cindex(completion system, variables)
+
+There are some standard variables, initialised by the tt(_main_complete)
+function and then used from other functions.
+
+The standard variables are:
+
+startitem()
+item(tt(_comp_caller_options))(
+The completion system uses tt(setopt) to set a number of options. This
+allows functions to be written without concern for compatibility with
+every possible combination of user options. However, sometimes completion
+needs to know what the user's option preferences are. These are saved
+in the tt(_comp_caller_options) associative array.
+)
+
+item(tt(_comp_priv_prefix))(
+Completion functions such as tt(_sudo) can set the tt(_comp_priv_prefix)
+array to a command that may then be used by tt(_call_program) to
+match the privileges when calling programs to generate matches.
+)
+enditem()
+
+Two more features are offered by the tt(_main_complete) function.  The
+arrays tt(compprefuncs) and tt(comppostfuncs) may contain
+names of functions that are to be called immediately before or after
+completion has been tried.  A function will only be called once unless
+it explicitly reinserts itself into the array.
+
+texinode(Completion Directories)()(Completion System Variables)(Completion System)
 sect(Completion Directories)
 cindex(completion system, directory structure)
 

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Daniel Shahaf]

Oliver Kiddle wrote on Sat, Sep 10, 2016 at 00:09:45 +0200:
> @@ -13,10 +20,10 @@ if zstyle -s ":completion:${curcontext}:${1}" command tmp; then
>    if [[ "$tmp" = -* ]]; then
>      eval "$tmp[2,-1]" "$argv[2,-1]"

Document somewhere that that _call_program -p overrides the 'command'
style if the latter's value starts with a hyphen?

>    else
> -    eval "$tmp"
> +    eval $prefix "$tmp"
>    fi
>  else
> -  eval "$argv[2,-1]"
> +  eval $prefix "$argv[2,-1]"
>  fi 2>&$err_fd

> +The standard variables are:
> +
> +startitem()
> +item(tt(_comp_caller_options))(
> +The completion system uses tt(setopt) to set a number of options. This
> +allows functions to be written without concern for compatibility with
> +every possible combination of user options. However, sometimes completion
> +needs to know what the user's option preferences are. These are saved
> +in the tt(_comp_caller_options) associative array.

As I said in 39168:

    What are the keys and values of the array?  Suggested text:
    
        The array maps option names, spelled in lowercase without
        underscores, to the string `tt(on)' or `tt(off)'.

> +item(tt(_comp_priv_prefix))(
> +Completion functions such as tt(_sudo) can set the tt(_comp_priv_prefix)
> +array to a command that may then be used by tt(_call_program) to
> +match the privileges when calling programs to generate matches.

I suggeted in 39168 the following:

s/command/prefix command/ in order to clarify to completion function
authors what they should set $_comp_priv_prefix to.

> +)

Cheers,

Daniel

Re: [PATCH] _virsh (Was: Re: zsh virsh completion)

[Oliver Kiddle]

On 4 Sep, Daniel Shahaf wrote:
> > > > It'd perhaps be useful if
> > > > the -p option to _call_program also caused it to add something to
> > > > $curcontext when looking up the command style but I'm not sure where
> > > > that could be done as we already have the tag and argument fields
> > > > filled. Any ideas?

> > Perhaps f/sudo? / can't appear in the command name.
> +1, sounds good.

This particular feature was missed before but is added with the
patch below. It is only added for the command and gain-privileges
styles, i.e. those checked by _call_program.

I also noticed that $words[1] was being updated by _arguments before the
code that used it so the patch fixes _sudo. I still need to test this
properly together with a real use such as virsh but I don't have virsh
or sudo installed on my normal system.

> How do we handle the case that >1 precommand wants to set _comp_priv_prefix?

At the moment, the last one sets it and overwrites it. Trying to
chain them is really going too far. Perhaps _sudo should just not
bother at all if _comp_priv_prefix is already set.

Oliver

diff --git a/Completion/Base/Utility/_call_program b/Completion/Base/Utility/_call_program
index 95c761e..9a44f2d 100644
--- a/Completion/Base/Utility/_call_program
+++ b/Completion/Base/Utility/_call_program
@@ -1,12 +1,15 @@
 #autoload +X
 
-local tmp err_fd=-1
+local curcontext="${curcontext}" tmp err_fd=-1
 local -a prefix
 
 if [[ "$1" = -p ]]; then
   shift
-  zstyle -t ":completion:${curcontext}:${1}" gain-privileges &&
-      prefix=( $_comp_priv_prefix )
+  if (( $#_comp_priv_prefix )); then
+    curcontext="${curcontext%:*}/${${(@M)_comp_priv_prefix:#^*[^\\]=*}[1]}:"
+    zstyle -t ":completion:${curcontext}:${1}" gain-privileges &&
+	prefix=( $_comp_priv_prefix )
+  fi
 fi
 
 if (( ${debug_fd:--1} > 2 )) || [[ ! -t 2 ]]
diff --git a/Completion/Solaris/Command/_pfexec b/Completion/Solaris/Command/_pfexec
index 3f1f3e7..2afaf31 100644
--- a/Completion/Solaris/Command/_pfexec
+++ b/Completion/Solaris/Command/_pfexec
@@ -25,7 +25,7 @@ _pfexec() {
  	_arguments \
 		'-P[privileges to acquire]:privspec:_privset' \
  		'(-):command name: _command_names -e' \
-		'*::arguments:{ _comp_priv_prefix=( $words[1] ${(kv)opt_args[-P]} ) ; _normal }'
+		'*::arguments:{ _comp_priv_prefix=( pfexec ${(kv)opt_args[-P]} ) ; _normal }'
 }
 
 _pfexec "$@"
diff --git a/Completion/Unix/Command/_sudo b/Completion/Unix/Command/_sudo
index 21b1ef4..0a212b7 100644
--- a/Completion/Unix/Command/_sudo
+++ b/Completion/Unix/Command/_sudo
@@ -2,7 +2,7 @@
 
 setopt localoptions extended_glob
 
-local environ e
+local environ e cmd
 local -a args
 
 zstyle -a ":completion:${curcontext}:" environ environ
@@ -39,6 +39,7 @@ args=(
 if [[ $service = sudoedit ]] || (( $words[(i)-e] < $words[(i)^(*sudo|-[^-]*)] ))  ; then
   args=( -A "-*" $args '!(-V --version -h --help)-e' '*:file:_files' )
 else
+  cmd="$words[1]"
   args+=(
     '(-e --edit 1 *)'{-e,--edit}'[edit files instead of running a command]' \
     '(-s --shell)'{-s,--shell}'[run shell as the target user; a command may also be specified]' \
@@ -48,7 +49,7 @@ else
     '(-H --set-home -i --login -s --shell -e --edit)'{-H,--set-home}"[set HOME variable to target user's home dir]" \
     '(-P --preserve-groups -i -login -s --shell -e --edit)'{-P,--preserve-groups}"[preserve group vector instead of setting to target's]" \
     '(-)1:command: _command_names -e'
-    '*::arguments:{ _comp_priv_prefix=( $words[1] -n ${(kv)opt_args[(I)(-[ugHEP]|--(user|group|set-home|preserve-env|preserve-groups))]} ) ; _normal }'
+    '*::arguments:{ _comp_priv_prefix=( $cmd -n ${(kv)opt_args[(I)(-[ugHEP]|--(user|group|set-home|preserve-env|preserve-groups))]} ) ; _normal }'
   )
 fi
 
diff --git a/Doc/Zsh/compsys.yo b/Doc/Zsh/compsys.yo
index cab665b..260ace4 100644
--- a/Doc/Zsh/compsys.yo
+++ b/Doc/Zsh/compsys.yo
@@ -4191,7 +4191,9 @@ output is influenced by the permissions it is run with. If the
 tt(gain-privileges) style is set to true, tt(_call_program) will make
 use of commands such as tt(sudo), if present on the command-line, to
 match the permissions to whatever the final command is likely to run
-under.
+under. When looking up the tt(gain-privileges) and tt(command) styles,
+the command component of the zstyle context will end with a slash
+(`tt(/)') followed by the command that would be used to gain privileges.
 )
 findex(_combination)
 item(tt(_combination) [ tt(-s) var(pattern) ] var(tag) var(style) var(spec) ... var(field) var(opts) ...)(