From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 12407 invoked from network); 27 Feb 2021 12:04:09 -0000 Received: from zero.zsh.org (2a02:898:31:0:48:4558:7a:7368) by inbox.vuxu.org with ESMTPUTF8; 27 Feb 2021 12:04:09 -0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=zsh.org; s=rsa-20200801; h=List-Archive:List-Owner:List-Post:List-Unsubscribe: List-Subscribe:List-Help:List-Id:Sender:Message-ID:Date: Content-Transfer-Encoding:Content-ID:Content-Type:MIME-Version:Subject:To: References:From:In-reply-to:cc:Reply-To:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID; bh=rooW8l5coBASBVkGR0mJre/KeKA4mr6io1HMxGU5zoY=; b=Sbc0RtMvoCn4c9Uh1QU25nHqTk yZjV5lEyYwNopkl9E1t48JEYlqWnjV5RCpHrynZ+BTfMd9mKHm3oZuHOT0fH6WLSzVppBCcPJpEiy dvI6Ie2Hk9n+XX39MkaFr0PP8vjH+6PWCc9kwfr0JQfgezF8fT22g4pzGFny8kqhfnGS8h3sbp8uR C05MuzGjKRpebsH/GwjVXWb4ztGRtSTc4ntnUyLdGma+KOyRqM4q8cudq3VcPRbj3mBB1lg85XmBD E6c/UdBvDOFnuABrkHMXCYzyCkwrowA4tl1ZHLuvf9TyYkduJcuATormlhaTdySDKN+2e/AGkmY2q GcXyw+JQ==; Received: from authenticated user by zero.zsh.org with local id 1lFyKO-000O5x-Di; Sat, 27 Feb 2021 12:04:08 +0000 Received: from authenticated user by zero.zsh.org with esmtpsa (TLS1.3:TLS_AES_256_GCM_SHA384:256) id 1lFyKB-000NwD-B0; Sat, 27 Feb 2021 12:03:55 +0000 Received: from [192.168.178.21] (helo=hydra) by mail.kiddle.eu with esmtp(Exim 4.93.0.4) (envelope-from ) id 1lFyK9-000C1m-4M; Sat, 27 Feb 2021 13:03:53 +0100 cc: zsh-workers@zsh.org In-reply-to: From: Oliver Kiddle References: <20210226075558.883716-1-ar@cs.msu.ru> <20210226155018.GF13554@tarpaulin.shahaf.local2> To: Arseny Maslennikov Subject: Re: [PATCH 1/2] Introduce new completion for Linux task capabilities MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-ID: <46236.1614427433.1@hydra> Content-Transfer-Encoding: 8bit Date: Sat, 27 Feb 2021 13:03:53 +0100 Message-ID: <46237-1614427433.132203@ES4F.fvp_.b2fX> X-Seq: 48123 Archived-At: X-Loop: zsh-workers@zsh.org Errors-To: zsh-workers-owner@zsh.org Precedence: list Precedence: bulk Sender: zsh-workers-request@zsh.org X-no-archive: yes List-Id: List-Help: List-Subscribe: List-Unsubscribe: List-Post: List-Owner: List-Archive: Archived-At: 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=». > > 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