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

[Arseny Maslennikov <ar@cs.msu.ru>]

[-- 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 --]