[PATCH 1/2] Introduce new completion for Linux task capabilities

[PATCH 1/2] Introduce new completion for Linux task capabilities

[Arseny Maslennikov]

This is intended for use on Linux-based systems only.

The next patch introduces a completion for setpriv(1), which actively
uses _capability_names. As for _capabilities, there are currently no
users of this function, but I believe some utilities that handle caps
may actually want to use it (neither setpriv(1) nor setcap/getcap(8),
for instance, want to offer the cap names themselves as completion
results; instead they want to prefix each name or a comma-separated
sequence of names), and if not yet — it is still available to be
manually compdeffed.
---
 Completion/Linux/Type/_capabilities     | 10 ++++++++++
 Completion/Linux/Type/_capability_names | 20 ++++++++++++++++++++
 2 files changed, 30 insertions(+)
 create mode 100644 Completion/Linux/Type/_capabilities
 create mode 100644 Completion/Linux/Type/_capability_names

diff --git a/Completion/Linux/Type/_capabilities b/Completion/Linux/Type/_capabilities
new file mode 100644
index 000000000..5a97aa335
--- /dev/null
+++ b/Completion/Linux/Type/_capabilities
@@ -0,0 +1,10 @@
+#autoload
+
+if [[ $OSTYPE != linux* ]]; then
+    _default; return
+fi
+
+local -a expl matches
+
+_capability_names matches
+_wanted capabilities expl "Linux capabilities" compadd "$@" -a - matches
diff --git a/Completion/Linux/Type/_capability_names b/Completion/Linux/Type/_capability_names
new file mode 100644
index 000000000..a2243b716
--- /dev/null
+++ b/Completion/Linux/Type/_capability_names
@@ -0,0 +1,20 @@
+#autoload
+
+_capability_names() {
+  # Stores an array of Linux task capability names under a parameter
+  # named by the first argument.
+
+  if [[ $OSTYPE != linux* ]]; then
+      _default; return
+  fi
+
+  # The list of Linux capabilities is taken from include/uapi/linux/capability.h
+  # and subject to the following pipe filter:
+  # grep 'define CAP' | sed -r 's/^[[:space:]]*#define[[:space:]]+//; s/[[:space:]]+[0-9]+$//' | tr '[[:upper:]]' '[[:lower:]]'
+  local -a C=(
+    cap_chown cap_dac_override cap_dac_read_search cap_fowner cap_fsetid cap_kill cap_setgid cap_setuid cap_setpcap cap_linux_immutable cap_net_bind_service cap_net_broadcast cap_net_admin cap_net_raw cap_ipc_lock cap_ipc_owner cap_sys_module cap_sys_rawio cap_sys_chroot cap_sys_ptrace cap_sys_pacct cap_sys_admin cap_sys_boot cap_sys_nice cap_sys_resource cap_sys_time cap_sys_tty_config cap_mknod cap_lease cap_audit_write cap_audit_control cap_setfcap cap_mac_override cap_mac_admin cap_syslog cap_wake_alarm cap_block_suspend cap_audit_read cap_perfmon cap_bpf cap_checkpoint_restore
+  )
+  set -A "$1" "${(@)C}"
+}
+
+_capability_names "$@"
-- 
2.30.1

[PATCH 2/2] Introduce new completion for setpriv(1) on Linux

[Arseny Maslennikov]

This is a utility from util-linux which sets or queries various Linux
process privilege settings that are inherited across execve(2). More
info is available in the corresponding manual page[1].

[1] https://man7.org/linux/man-pages/man1/setpriv.1.html
---
 Completion/Linux/Command/_setpriv | 88 +++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 Completion/Linux/Command/_setpriv

diff --git a/Completion/Linux/Command/_setpriv b/Completion/Linux/Command/_setpriv
new file mode 100644
index 000000000..8fdd2ed10
--- /dev/null
+++ b/Completion/Linux/Command/_setpriv
@@ -0,0 +1,88 @@
+#compdef setpriv
+
+__setpriv_prctl_securebits_set_element() {
+  local -a expl matches
+  local -a bits
+
+  bits=(noroot noroot_locked
+        no_setuid_fixup no_setuid_fixup_locked
+        keep_caps_locked
+  )
+  matches=( {-,+}"${(@)^bits}" )
+  _wanted minus-plus-securebits expl 'prctl securebits' \
+    compadd "$@" -a - matches
+}
+
+__setpriv_prctl_securebits_set() {
+  _sequence __setpriv_prctl_securebits_set_element
+}
+
+__setpriv_capability_expressions() {
+  # Nonlocal expl; _description call expected.
+  local -a caps matches
+
+  _capability_names caps
+  # Strip the prefix "cap_" from every array element.
+  # For every element, prepend "-" and "+" to the element.
+  matches=( {-,+}"${(@)^caps#cap_}" )
+  compadd "$@" "${(@)expl}" -a - matches
+}
+
+__setpriv_caps_all() {
+  # Nonlocal expl; _description call expected.
+  local -a names matches
+
+  names=(all)
+  matches=( {-,+}"${(@)^names}" )
+  compadd "$@" "${(@)expl}" -a - matches
+}
+
+__setpriv_cap_set_element() {
+  # We pass through arguments from _sequence.
+  local -a Oargv=( "$@" )
+  _alternative -O Oargv \
+    'special-actions:drop/obtain all caps:__setpriv_caps_all' \
+    'minus-plus-caps:capabilities:__setpriv_capability_expressions' \
+    #
+}
+
+__setpriv_cap_set() {
+  _sequence __setpriv_cap_set_element
+}
+
+__setpriv_death_signals() {
+  _alternative \
+    'special-actions:keep or clear:(keep clear)' \
+    'signals:UNIX signals:_signals' \
+    #
+}
+
+local context state state_descr line
+typeset -A opt_args
+
+_arguments -S \
+  '(- : *)--help[print help and exit]' \
+  '(- : *)'{-V,--version}'[print version information and exit]' \
+  '(- : *)*'{-d,--dump}'[display the current privilege state]' \
+  '--clear-groups[clear supplementary groups]' \
+  '--groups[set supplementary groups]:groups:_groups' \
+  '--inh-caps[set inheritable caps]:capability set: __setpriv_cap_set' \
+  '--ambient-caps[set ambient caps]:capability set: __setpriv_cap_set' \
+  '--bounding-set[set the cap bounding set]:capability set: __setpriv_cap_set' \
+  '(- : *)--list-caps[list all known capabilities]' \
+  '--keep-groups[preserve supplementary groups]' \
+  '--init-groups[initialize supplementary groups]' \
+  '--no-new-privs[set NO_NEW_PRIVS]' \
+  '--rgid[set real UNIX group id]:UNIX group:_groups' \
+  '--egid[set effective UNIX group id]:UNIX group:_groups' \
+  '--regid[set real and effective UNIX group id]:UNIX group:_groups' \
+  '--ruid[set real UNIX user id]:UNIX user:_users' \
+  '--euid[set effective UNIX user id]:UNIX user:_users' \
+  '--reuid[set real and effective UNIX user id]:UNIX user:_users' \
+  '--securebits[set "process securebits"]:prctl securebits:__setpriv_prctl_securebits_set' \
+  '--pdeathsig[keep, clear, or set parent death signal]:signals: __setpriv_death_signals' \
+  '--selinux-label[request a selinux label]:SELinux labels: ' \
+  '--apparmor-profile[request an apparmor profile]:AppArmor profiles: ' \
+  '--reset-env[set environment as for a classic login shell]' \
+  '*:::command:_normal' \
+  #
-- 
2.30.1

Re: [PATCH 1/2] Introduce new completion for Linux task capabilities

[Daniel Shahaf]

Arseny Maslennikov wrote on Fri, Feb 26, 2021 at 10:55:57 +0300:
> The next patch introduces a completion for setpriv(1), which actively
> uses _capability_names. As for _capabilities,

(The difference is _capability_names just returns the names in an
array, while _capabilities actually adds them as completions.)

> there are currently no
> users of this function, but I believe some utilities that handle caps
> may actually want to use it (neither setpriv(1) nor setcap/getcap(8),
> for instance, want to offer the cap names themselves as completion
> results; instead they want to prefix each name or a comma-separated
> sequence of names),

FWIW, I think it may well be possible to write _setpriv in terms of
_capabilities as a black box, using some combination of _sequence,
matchspecs, «compadd -P», et al..

As to setcap(8), it uses cap_from_text(3), which is a more elaborate
format that presumably deserves its own completion function, to be used
by any tool that uses that library function.

To be clear, I'm not asking for anything to be changed.  (Others might,
of course.)

> and if not yet — it is still available to be manually compdeffed.

So you pronounce "compdef" with the stress on the second syllable?

> +++ b/Completion/Linux/Type/_capabilities
> @@ -0,0 +1,10 @@
> +#autoload
> +
> +if [[ $OSTYPE != linux* ]]; then
> +    _default; return
> +fi

Why?  The code doesn't use /proc or /sbin/some-linux-only-tool, and
having this guard prevents people on other OSTYPE's from completing «ssh
$linuxbox foo --with-capability=<TAB>».

> +++ b/Completion/Linux/Type/_capability_names
> @@ -0,0 +1,20 @@
> +#autoload
> +
> +_capability_names() {
> +  # Stores an array of Linux task capability names under a parameter
> +  # named by the first argument.
> +
> +  # The list of Linux capabilities is taken from include/uapi/linux/capability.h
> +  # and subject to the following pipe filter:
> +  # grep 'define CAP' | sed -r 's/^[[:space:]]*#define[[:space:]]+//; s/[[:space:]]+[0-9]+$//' | tr '[[:upper:]]' '[[:lower:]]'
> +  local -a C=(

Please use a longer, meaningful variable name.

> +    cap_chown cap_dac_override cap_dac_read_search cap_fowner cap_fsetid cap_kill cap_setgid cap_setuid cap_setpcap cap_linux_immutable cap_net_bind_service cap_net_broadcast cap_net_admin cap_net_raw cap_ipc_lock cap_ipc_owner cap_sys_module cap_sys_rawio cap_sys_chroot cap_sys_ptrace cap_sys_pacct cap_sys_admin cap_sys_boot cap_sys_nice cap_sys_resource cap_sys_time cap_sys_tty_config cap_mknod cap_lease cap_audit_write cap_audit_control cap_setfcap cap_mac_override cap_mac_admin cap_syslog cap_wake_alarm cap_block_suspend cap_audit_read cap_perfmon cap_bpf cap_checkpoint_restore

Suggest to list these one per line, because:

- Some interfaces (e.g., https://github.com/zsh-users/zsh/) support
  line-based blame but not word-based blame (`git log -S`).

- If capabilities are ever removed, the diff removing them would be
  clearer.

In terms of the filter, that's just «| xargs -n1».

> +  )
> +  set -A "$1" "${(@)C}"

Handle the case that no arguments were passed — e.g., by doing ${1:-reply}.

I'll leave reviewing 2/2 to someone else.

Thanks for the patch,

Daniel


> +}
> +
> +_capability_names "$@"
> -- 
> 2.30.1
> 
> 

Re: [PATCH 1/2] Introduce new completion for Linux task capabilities

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 4771 bytes --]

On Fri, Feb 26, 2021 at 03:50:18PM +0000, Daniel Shahaf wrote:
> Arseny Maslennikov wrote on Fri, Feb 26, 2021 at 10:55:57 +0300:
> > The next patch introduces a completion for setpriv(1), which actively
> > uses _capability_names. As for _capabilities,
> 
> (The difference is _capability_names just returns the names in an
> array, while _capabilities actually adds them as completions.)
> 
> > there are currently no
> > users of this function, but I believe some utilities that handle caps
> > may actually want to use it (neither setpriv(1) nor setcap/getcap(8),
> > for instance, want to offer the cap names themselves as completion
> > results; instead they want to prefix each name or a comma-separated
> > sequence of names),
> 
> FWIW, I think it may well be possible to write _setpriv in terms of
> _capabilities as a black box, using some combination of _sequence,
> matchspecs, «compadd -P», et al..

Do you mean invoking «_capabilities -O array_name» to pass the -O to
compadd, and then using the array_name in _setpriv and possible future
users?
As far as I understand, in that case _capabilities will have to avoid
looping over tag labels, i. e. calling _wanted, as it does now.

> 
> As to setcap(8), it uses cap_from_text(3), which is a more elaborate
> format that presumably deserves its own completion function, to be used
> by any tool that uses that library function.

I'm likely not ready to write one just yet, but I agree.

> 
> To be clear, I'm not asking for anything to be changed.  (Others might,
> of course.)
> 
> > and if not yet — it is still available to be manually compdeffed.
> 
> So you pronounce "compdef" with the stress on the second syllable?

Yes, I do.
Never had the chance to hear the word in oral speech, though. :)

Come to think of it, considering the "comp" prefix is quite common in
our completion system, it doesn't make a lot of sense to stress "comp".

> 
> > +++ b/Completion/Linux/Type/_capabilities
> > @@ -0,0 +1,10 @@
> > +#autoload
> > +
> > +if [[ $OSTYPE != linux* ]]; then
> > +    _default; return
> > +fi
> 
> Why?  The code doesn't use /proc or /sbin/some-linux-only-tool, and
> having this guard prevents people on other OSTYPE's from completing «ssh
> $linuxbox foo --with-capability=<TAB>».

I intended to play nice in case unrelated "capabilities" are present (or
introduced later) on those OSTYPE's and pulled Oliver's trick from the
recently posted _unshare. There might be a better way to do it; I'm open
to discussion.

> 
> > +++ b/Completion/Linux/Type/_capability_names
> > @@ -0,0 +1,20 @@
> > +#autoload
> > +
> > +_capability_names() {
> > +  # Stores an array of Linux task capability names under a parameter
> > +  # named by the first argument.
> > +
> > +  # The list of Linux capabilities is taken from include/uapi/linux/capability.h
> > +  # and subject to the following pipe filter:
> > +  # grep 'define CAP' | sed -r 's/^[[:space:]]*#define[[:space:]]+//; s/[[:space:]]+[0-9]+$//' | tr '[[:upper:]]' '[[:lower:]]'

A nitpick to myself:
For simplicity I did not mention that this filter still appends between 3 and
6 lines to the desired output; they are of technical value and of no
use to us since they do not contain cap names, and can be stripped manually.

> > +  local -a C=(
> 
> Please use a longer, meaningful variable name.

OK.

> 
> > +    cap_chown cap_dac_override cap_dac_read_search cap_fowner cap_fsetid cap_kill cap_setgid cap_setuid cap_setpcap cap_linux_immutable cap_net_bind_service cap_net_broadcast cap_net_admin cap_net_raw cap_ipc_lock cap_ipc_owner cap_sys_module cap_sys_rawio cap_sys_chroot cap_sys_ptrace cap_sys_pacct cap_sys_admin cap_sys_boot cap_sys_nice cap_sys_resource cap_sys_time cap_sys_tty_config cap_mknod cap_lease cap_audit_write cap_audit_control cap_setfcap cap_mac_override cap_mac_admin cap_syslog cap_wake_alarm cap_block_suspend cap_audit_read cap_perfmon cap_bpf cap_checkpoint_restore
> 
> Suggest to list these one per line, because:
> 
> - Some interfaces (e.g., https://github.com/zsh-users/zsh/) support
>   line-based blame but not word-based blame (`git log -S`).
> 
> - If capabilities are ever removed, the diff removing them would be
>   clearer.
> 
> In terms of the filter, that's just «| xargs -n1».

I'd personally like them being listed one per line, too. Will do.

> 
> > +  )
> > +  set -A "$1" "${(@)C}"
> 
> Handle the case that no arguments were passed — e.g., by doing ${1:-reply}.

OK.

> 
> I'll leave reviewing 2/2 to someone else.

Ok; I'm going to send a revised series once 2/2 is reviewed in case
there are any problems with it.

> 
> Thanks for the patch,
> 
> Daniel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] Introduce new completion for Linux task capabilities

[Oliver Kiddle]

Arseny Maslennikov wrote:
> On Fri, Feb 26, 2021 at 03:50:18PM +0000, Daniel Shahaf wrote:
> > Arseny Maslennikov wrote on Fri, Feb 26, 2021 at 10:55:57 +0300:
> > > The next patch introduces a completion for setpriv(1), which actively

Thanks for the contribution. I've reviewed the second patch that Daniel
left though in general it all looks very good and there isn't much need
to comment.

> > > uses _capability_names. As for _capabilities,

Are these POSIX capabilities? The term capabilities gets wider usage so
I'd be inclined to call the function _posix_capabilities despite that
being longer. As far as I can tell, unless you count Hurd, Linux is the
only implementation of POSIX capabilities so it is fine for this
to be in the Linux directory. That said, I'm not sure I would factor it
out of _setpriv until there's at least two users of it.

> > FWIW, I think it may well be possible to write _setpriv in terms of
> > _capabilities as a black box, using some combination of _sequence,
> > matchspecs, «compadd -P», et al..
>
> Do you mean invoking «_capabilities -O array_name» to pass the -O to
> compadd, and then using the array_name in _setpriv and possible future
> users?
> As far as I understand, in that case _capabilities will have to avoid
> looping over tag labels, i. e. calling _wanted, as it does now.

No. You would call _capabilities with compadd style options to deal with
the prefixes. You have the following:

  matches=( {-,+}"${(@)^caps#cap_}" )

So _capabilities is unsuited to _setpriv because you don't want the
cap_ prefix but do want - and + prefixes. Telling compadd/completion
functions to strip a prefix is somewhat messy, you can get close with
matching control (-M 'B:=cap_') but it continues to prefer the prefix
to be present. So I'd suggest that you chop off the initial "cap_" from
everything _capabilites completes.

Adding prefixes is easier. If some other function needs capabilities
with a cap_ prefix, that can then call it with -p 'cap_',

For the - and +, we can also add these as a prefix but in their case, -P
is more appropriate because the prefix is a separate thing and it may
even be better to strip them with compset and complete them
independently so that we can indicate that they are for removing and
adding capabilities.

> > > +if [[ $OSTYPE != linux* ]]; then
> > > +    _default; return
> > > +fi
> > 
> > Why?  The code doesn't use /proc or /sbin/some-linux-only-tool, and
> > having this guard prevents people on other OSTYPE's from completing «ssh
> > $linuxbox foo --with-capability=<TAB>».
>
> I intended to play nice in case unrelated "capabilities" are present (or
> introduced later) on those OSTYPE's and pulled Oliver's trick from the
> recently posted _unshare. There might be a better way to do it; I'm open
> to discussion.

Not sure I'd bother in this case as - given the lack of setpriv commands
on other operating systems - it is unlikely to be called other than in
a case like the ssh one Daniel mentions. It mattered in the case of
unshare because unshare does exist on other OSes as an unrelated command
to remove NFS exports.

> > Suggest to list these one per line, because:
> I'd personally like them being listed one per line, too. Will do.

I'd certainly avoid exceeding 80 columns unnecessarily but if it ends up
using a lot of vertical space, grouping similar ones or by first letter
is also fine.

Arseny Maslennikov [patch 2/2] wrote:
> +__setpriv_prctl_securebits_set_element() {

I wouldn't bother with the double-underscore prefix. _git did that and
seems to have been copied elsewhere but it doesn't really serve much
purpose. I think with git it was done to make a more obvious distinction
to subcommands than just whether it was _git_… or _git-…

I'd be inclined to just name it _setpriv_prctl_securebits as that is
what it appears to complete.

> +  bits=(noroot noroot_locked
> +        no_setuid_fixup no_setuid_fixup_locked

> +  _wanted minus-plus-securebits expl 'prctl securebits' \
> +    compadd "$@" -a - matches

Minor nitpick but Etc/completion-style-guide states four spaces of
indentation for continuation lines. My own personal taste includes a
particular dislike for the variable indentation that results from lining
things up with the syntactic construct on the previous line as in the
bits=( assignment.

Also, the description should be in the singular ('prctl securebit').
Yes, there may be multiple matches, and we may be in the middle of a
comma-separated list of them but only one is completed at a time

T'd be sufficient to use _description followed by compadd rather than
_wanted. There are aspects of nested tag loops that aren't ideal so
when you can know that completion is already within one tag loop (which
_arguments does), we don't need another one.

> +__setpriv_prctl_securebits_set() {
> +  _sequence __setpriv_prctl_securebits_set_element
> +}

You can call _sequence directly from the _arguments spec. I'd probably
just do that rather than having a separate function. Convention for
separate functions is a plural name, i.e. …_sets

> +__setpriv_caps_all() {
> +  # Nonlocal expl; _description call expected.

Is that necessary? Aren't descriptions passed through with "$@"?

> +__setpriv_cap_set_element() {
> +  # We pass through arguments from _sequence.
> +  local -a Oargv=( "$@" )
> +  _alternative -O Oargv \
> +    'special-actions:drop/obtain all caps:__setpriv_caps_all' \

Wouldn't :(-all +all) do for the action here instead of needing another
separate function.
There probably ought to be a nicer way to add special all/any elements
to a list because it comes up very often.

> +    'minus-plus-caps:capabilities:__setpriv_capability_expressions' \

Again, use singular form for the description.

> +local context state state_descr line
> +typeset -A opt_args

Given that you've not used states, these are superfluous.

> +  '(- : *)*'{-d,--dump}'[display the current privilege state]' \

The exclusion list here breaks the repeatability.
I did once look into making a special case for this in _arguments but it
wasn't trivial.

> +  '--clear-groups[clear supplementary groups]' \
> +  '--groups[set supplementary groups]:groups:_groups' \

How do you specify multiple supplementary groups? If --groups is
repeated, we need * at the beginning of the spec. Otherwise, is
_sequence needed.
Are there any mutual exclusions between it and --clear-groups,
--keep-groups and --init-groups.

> +  '--inh-caps[set inheritable caps]:capability set: __setpriv_cap_set' \
> +  '--ambient-caps[set ambient caps]:capability set: __setpriv_cap_set' \

Where the _arguments descriptions are being thrown out anyway, it may be
better to leave them out entirely and just have a single space.

> +  '--securebits[set "process securebits"]:prctl securebits:__setpriv_prctl_securebits_set' \

Starting with this one is a run of plural descriptions.

Oliver

Re: [PATCH 1/2] Introduce new completion for Linux task capabilities

[Lawrence Velázquez]

Hi Arseny,

On Sat, Feb 27, 2021, at 7:03 AM, Oliver Kiddle wrote:
> Arseny Maslennikov wrote:
> > On Fri, Feb 26, 2021 at 03:50:18PM +0000, Daniel Shahaf wrote:
> > > Arseny Maslennikov wrote on Fri, Feb 26, 2021 at 10:55:57 +0300:
> > > > The next patch introduces a completion for setpriv(1), which actively
> 
> Thanks for the contribution.

Will you be addressing Oliver's feedback?

vq

Re: [PATCH 1/2] Introduce new completion for Linux task capabilities

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Fri, Mar 19, 2021 at 10:43:20PM -0400, Lawrence Velázquez wrote:
> Hi Arseny,
> 
> On Sat, Feb 27, 2021, at 7:03 AM, Oliver Kiddle wrote:
> > Arseny Maslennikov wrote:
> > > On Fri, Feb 26, 2021 at 03:50:18PM +0000, Daniel Shahaf wrote:
> > > > Arseny Maslennikov wrote on Fri, Feb 26, 2021 at 10:55:57 +0300:
> > > > > The next patch introduces a completion for setpriv(1), which actively
> > 
> > Thanks for the contribution.
> 
> Will you be addressing Oliver's feedback?

Yes, I was a bit hesitant on how to deal with some points raised by
Oliver. I'm sending v2 shortly.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] Introduce new completion for Linux task capabilities

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 10675 bytes --]

On Sat, Feb 27, 2021 at 01:03:53PM +0100, Oliver Kiddle wrote:
> Arseny Maslennikov wrote:
> > On Fri, Feb 26, 2021 at 03:50:18PM +0000, Daniel Shahaf wrote:
> > > Arseny Maslennikov wrote on Fri, Feb 26, 2021 at 10:55:57 +0300:
> > > > The next patch introduces a completion for setpriv(1), which actively
> 
> Thanks for the contribution. I've reviewed the second patch that Daniel
> left though in general it all looks very good and there isn't much need
> to comment.
> 
> > > > uses _capability_names. As for _capabilities,
> 
> Are these POSIX capabilities?

The corresponding POSIX draft standard was actually withdrawn.

Even if we imagine it wasn't, however, as time passes, new capabilities
are regularly being introduced to Linux, and I'm not sure if POSIX
capabilities would reflect the additions soon enough for the supported
capability sets to not diverge significantly. It's not like Linux
development follows POSIX, but the other way around — the bits that make
their way to popular Unix-like systems are codified as parts of the
POSIX standard.

> The term capabilities gets wider usage so
> I'd be inclined to call the function _posix_capabilities despite that
> being longer. As far as I can tell, unless you count Hurd, Linux is the
> only implementation of POSIX capabilities so it is fine for this
> to be in the Linux directory. That said, I'm not sure I would factor it
> out of _setpriv until there's at least two users of it.

After pondering this question, I get your point. I'm now inclined to
agree with you on the _posix_capabilities name, since it clearly conveys
what kind of capabilities the completion function is about, if and only
if the aforementioned POSIX draft is resurrected in some way.

If «_capabilities» is still too unclear, I suggest «_task_capabilities».

> 
> > > FWIW, I think it may well be possible to write _setpriv in terms of
> > > _capabilities as a black box, using some combination of _sequence,
> > > matchspecs, «compadd -P», et al..
> >
> > Do you mean invoking «_capabilities -O array_name» to pass the -O to
> > compadd, and then using the array_name in _setpriv and possible future
> > users?
> > As far as I understand, in that case _capabilities will have to avoid
> > looping over tag labels, i. e. calling _wanted, as it does now.
> 
> No. You would call _capabilities with compadd style options to deal with
> the prefixes. You have the following:
> 
>   matches=( {-,+}"${(@)^caps#cap_}" )
> 
> So _capabilities is unsuited to _setpriv because you don't want the
> cap_ prefix but do want - and + prefixes. Telling compadd/completion
> functions to strip a prefix is somewhat messy, you can get close with
> matching control (-M 'B:=cap_') but it continues to prefer the prefix
> to be present. So I'd suggest that you chop off the initial "cap_" from
> everything _capabilites completes.
> 
> Adding prefixes is easier. If some other function needs capabilities
> with a cap_ prefix, that can then call it with -p 'cap_',
> 
> For the - and +, we can also add these as a prefix but in their case, -P
> is more appropriate because the prefix is a separate thing and it may
> even be better to strip them with compset and complete them
> independently so that we can indicate that they are for removing and
> adding capabilities.

OK, I managed to make this work with «compset -P [+-]» and eliminate the
need for _capability_names; I'm going to send the revised patch shortly.
Thanks a lot for the thorough explanation!

> 
> > > > +if [[ $OSTYPE != linux* ]]; then
> > > > +    _default; return
> > > > +fi
> > > 
> > > Why?  The code doesn't use /proc or /sbin/some-linux-only-tool, and
> > > having this guard prevents people on other OSTYPE's from completing «ssh
> > > $linuxbox foo --with-capability=<TAB>».
> >
> > I intended to play nice in case unrelated "capabilities" are present (or
> > introduced later) on those OSTYPE's and pulled Oliver's trick from the
> > recently posted _unshare. There might be a better way to do it; I'm open
> > to discussion.
> 
> Not sure I'd bother in this case as - given the lack of setpriv commands
> on other operating systems - it is unlikely to be called other than in
> a case like the ssh one Daniel mentions. It mattered in the case of
> unshare because unshare does exist on other OSes as an unrelated command
> to remove NFS exports.

OK, I'll remove that check until this is a problem.

> 
> > > Suggest to list these one per line, because:
> > I'd personally like them being listed one per line, too. Will do.
> 
> I'd certainly avoid exceeding 80 columns unnecessarily but if it ends up
> using a lot of vertical space, grouping similar ones or by first letter
> is also fine.
> 
> Arseny Maslennikov [patch 2/2] wrote:
> > +__setpriv_prctl_securebits_set_element() {
> 
> I wouldn't bother with the double-underscore prefix. _git did that and
> seems to have been copied elsewhere but it doesn't really serve much
> purpose. I think with git it was done to make a more obvious distinction
> to subcommands than just whether it was _git_… or _git-…

I wanted that prefix to denote a quasi-namespace of sorts, since zsh
does not have a concept of local functions. E. g.:
* _setpriv is a "front-end" completion function, as is _git, _signals, ...
  One underscore.
* __setpriv_death_signals is an implementation detail of _setpriv that
  completes a death signal and depends on how it is called (is it
  expected to call _description? is it expected to loop over labels?), so
  it has _setpriv directly in the name after the initial underscore.

Something like this was my initial understanding of how _git and all the
functions that followed suit use __.

> 
> I'd be inclined to just name it _setpriv_prctl_securebits as that is
> what it appears to complete.

This would be an appropriate name if it generated matches from the bits
array, i. e. the securebit names themselves as matches. Instead, the
function has to append '-' or '+' to each securebit name.

> 
> > +  bits=(noroot noroot_locked
> > +        no_setuid_fixup no_setuid_fixup_locked
> 
> > +  _wanted minus-plus-securebits expl 'prctl securebits' \
> > +    compadd "$@" -a - matches
> 
> Minor nitpick but Etc/completion-style-guide states four spaces of
> indentation for continuation lines. My own personal taste includes a
> particular dislike for the variable indentation that results from lining
> things up with the syntactic construct on the previous line as in the
> bits=( assignment.
> 
> Also, the description should be in the singular ('prctl securebit').
> Yes, there may be multiple matches, and we may be in the middle of a
> comma-separated list of them but only one is completed at a time.

OK, I'm going to fix all the description strings to be in the singular.
To be fair, in the wild and likely even in the completion functions
bundled with zsh I've seen both plural and singular forms there.

> 
> T'd be sufficient to use _description followed by compadd rather than
> _wanted. There are aspects of nested tag loops that aren't ideal so
> when you can know that completion is already within one tag loop (which
> _arguments does), we don't need another one.
> 
> > +__setpriv_prctl_securebits_set() {
> > +  _sequence __setpriv_prctl_securebits_set_element
> > +}
> 
> You can call _sequence directly from the _arguments spec. I'd probably
> just do that rather than having a separate function. Convention for

I do not really have a preference for separate functions or inlining
calls/matches in the optspec. Separate function names, though, are
particularly easy to locate in the xtraced output generated by
_complete_debug, and they're often shorter than the one-liner they
implement so the separate name makes the optspec line easier to fit in
80 cols or whatever's the limit, so they got that going for them.

> separate functions is a plural name, i.e. …_sets

So «: : _sequence __setpriv_prctl_securebit_set_elements». That's all
right to me, but the full line for --securebits would be 97 columns
long.
The longest line is 105, so not really a problem, I guess...

> 
> > +__setpriv_caps_all() {
> > +  # Nonlocal expl; _description call expected.
> 
> Is that necessary? Aren't descriptions passed through with "$@"?

I checked with _complete_debug; they indeed are.

> 
> > +__setpriv_cap_set_element() {
> > +  # We pass through arguments added by _sequence.
> > +  local -a Oargv=( "$@" )
> > +  _alternative -O Oargv \
> > +    'special-actions:drop/obtain all caps:__setpriv_caps_all' \
> 
> Wouldn't :(-all +all) do for the action here instead of needing another
> separate function.
> There probably ought to be a nicer way to add special all/any elements
> to a list because it comes up very often.
> 
> > +    'minus-plus-caps:capabilities:__setpriv_capability_expressions' \
> 
> Again, use singular form for the description.
> 
> > +local context state state_descr line
> > +typeset -A opt_args
> 
> Given that you've not used states, these are superfluous.
> 
> > +  '(- : *)*'{-d,--dump}'[display the current privilege state]' \
> 
> The exclusion list here breaks the repeatability.
> I did once look into making a special case for this in _arguments but it
> wasn't trivial.

The cleanest way to fix this that comes to my mind is to introduce a
state here and call
    _arguments '*'{-d,--dump}'[display the current privilege state]'
from the handler.
I'll send that as a separate patch in the series.

> 
> > +  '--clear-groups[clear supplementary groups]' \
> > +  '--groups[set supplementary groups]:groups:_groups' \
> 
> How do you specify multiple supplementary groups? If --groups is
> repeated, we need * at the beginning of the spec. Otherwise, is
> _sequence needed.

I forgot to use _sequence there.

> Are there any mutual exclusions between it and --clear-groups,
> --keep-groups and --init-groups.

Turns out they all mutually exclude each other. Added.

> 
> > +  '--inh-caps[set inheritable caps]:capability set: __setpriv_cap_set' \
> > +  '--ambient-caps[set ambient caps]:capability set: __setpriv_cap_set' \
> 
> Where the _arguments descriptions are being thrown out anyway, it may be
> better to leave them out entirely and just have a single space.

Done everywhere in the code.

> 
> > +  '--securebits[set "process securebits"]:prctl securebits:__setpriv_prctl_securebits_set' \
> 
> Starting with this one is a run of plural descriptions.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]