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=-2.2 required=5.0 tests=DKIM_ADSP_ALL,DKIM_SIGNED, DKIM_VALID,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 1706 invoked from network); 21 Mar 2021 12:54:43 -0000 Received: from zero.zsh.org (2a02:898:31:0:48:4558:7a:7368) by inbox.vuxu.org with ESMTPUTF8; 21 Mar 2021 12:54:43 -0000 ARC-Seal: i=1; cv=none; a=rsa-sha256; d=zsh.org; s=rsa-20200801; t=1616331283; b=trCJ8oYfmOrx4KOh4nbgakHW42z2KZQZ3zCdyV7XnC7A6TL5RY6WpLVeWxGgwwyfkb4N8KwOtw 37eYRDVzix2TIaUCmZTn4NtrLWPcJ90reobSZpo5mUS9J7kJxLYvTE7zjK7waTCUv1kPS+gTZi DTjupRtYaSynBNkXP4lNsF1AFUcVMaTOLYdP/5MkqY6IkGaKe4cGqoBHAB3S7BalOJ7xmFZa4m 1mrkJoI0YKW4Mu5DFY37Cv3xgI62jXBBF9naJhg8OWnq/oP8pzttVL/3gaa9mJQflp28q3hsGF sJ1qJt1ccvUiYB8PV4sc35JrdvSoIEWBQYbq5HDvfIkunA==; ARC-Authentication-Results: i=1; zsh.org; iprev=pass (mx.cs.msu.ru) smtp.remote-ip=188.44.42.42; dkim=pass header.d=cs.msu.ru header.s=dkim header.a=rsa-sha256; dmarc=pass header.from=cs.msu.ru; arc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed; d=zsh.org; s=rsa-20200801; t=1616331283; bh=6vh2H8RycWjbz3ldp1JUFn0JjPzM1t+tzL8lDaGi+GI=; h=List-Archive:List-Owner:List-Post:List-Unsubscribe:List-Subscribe:List-Help: List-Id:Sender:Subject:In-Reply-To:Content-Type:MIME-Version:References: Message-ID:Cc:To:From:Date:DKIM-Signature:DKIM-Signature; b=x83nNAN0/LV72oH6Ttr2R407Geit0q+nRuEFvV3NpXy5B3nXJ8V2mhGEQYxZtO3lICVDHIJZBm QE5ZaPtHLqkGIMZy25xdmd/jAbhzPoZdqynaJPf4tAvmCc13sf9uMRpAKv2CFipG4uxiuQneGd GFlKxbhiDTjr5pnUsnukY3v3Lg/qvARSYbV5+ab6pkXVkVr7qjwLYwBWF3mrFqCeRNHoDw4nEg tmLFYNc+49F62LS/BevGxMVdccLDwMU9fQeuGZ4D3xQag6OjCL+l9rrEfablgGM6sla8RFHEit 5W4iXz5GpTqwPBbYPuDp4VU33F1VBNRwoQtdGcdQd5Kupw==; 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:Subject:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID; bh=zrQDf/iOGXAG/I+p28Hl031NO6rLNzenKPrUg5D4wSM=; b=jBQAhSefoIvJTEwFxhhT44KB+r klwYs9PW+/QQBP2OyHBdSJ9YD479J4XR/6krie/auaVXhLZF+iRoZ8RUBxNONcsfxWzVpwovSgRB1 7xEppJKZC1d2v/LSxo9KQLhS1cie/LdViemzgmp6P929IEQaMUI796FpZUMuxJ0QVFmwFK682O7Q1 xOMY3fuIS8GVQTPQFm/WvYJKHUPFQJt4iap6wwmtp5487UUAqWKncl7hcGxoo1xqNND4SqkwClPpI bABSVoFTUCHM2iwiiGmvCsFbOcsLL6vrSNr35n1DYQjSw9gzOIXPe442PgPueMDfvByH3PItrGYsK iGB8VhYg==; Received: from authenticated user by zero.zsh.org with local id 1lNxbM-0002PM-Ie; Sun, 21 Mar 2021 12:54:40 +0000 Authentication-Results: zsh.org; iprev=pass (mx.cs.msu.ru) smtp.remote-ip=188.44.42.42; dkim=pass header.d=cs.msu.ru header.s=dkim header.a=rsa-sha256; dmarc=pass header.from=cs.msu.ru; arc=none Received: from mx.cs.msu.ru ([188.44.42.42]:51402 helo=mail.cs.msu.ru) by zero.zsh.org with esmtps (TLS1.2:ECDHE-RSA-AES256-GCM-SHA384:256) id 1lNxb6-0002F4-Ri; Sun, 21 Mar 2021 12:54:26 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=cs.msu.ru; s=dkim; h=Subject:In-Reply-To:Content-Type:MIME-Version:References:Message-ID :Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=zrQDf/iOGXAG/I+p28Hl031NO6rLNzenKPrUg5D4wSM=; b=jID2RBey0vzryfaD8iDHYN8qac pk2zuv1Y0L9uxsarXy3I69e0jjtO9o/aa+k+0EsJKbEZNHhHRLDoXVtihim2Ub9cHESiPTZ7/sci+ FnPwwTRNoyvhaFjBUDxYRcIUPPBPyHayU95SxWB/OhSMZkMrgd1+WDShK/0CREPJNR3BWPkn6GZPT JZgu6fQslBBjMCDbYS/b31lKkesBhkhp59fPmFjMqmLtHezTktqcbvsEEpnth82PNlHuf1ZamOE9H K3fBy+FgrNRWSfJzikm8Wl4plNX3gGQ+c8H3GqRUiggfo6vvv95mVVlQoqeaagHnLIhZo/jqjBCx3 dzNmsCaA==; Received: from [37.204.119.143] (port=37736 helo=cello) by mail.cs.msu.ru with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94 (FreeBSD)) (envelope-from ) id 1lNxb4-000F2k-Fz; Sun, 21 Mar 2021 15:54:23 +0300 Date: Sun, 21 Mar 2021 15:54:16 +0300 From: Arseny Maslennikov To: Oliver Kiddle Cc: zsh-workers@zsh.org Message-ID: References: <20210226075558.883716-1-ar@cs.msu.ru> <20210226155018.GF13554@tarpaulin.shahaf.local2> <46237-1614427433.132203@ES4F.fvp_.b2fX> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="sOlSqEoF9c4nUQtT" Content-Disposition: inline In-Reply-To: <46237-1614427433.132203@ES4F.fvp_.b2fX> OpenPGP: url=http://grep.cs.msu.ru/~ar/pgp-key.asc X-SA-Exim-Connect-IP: 37.204.119.143 X-SA-Exim-Mail-From: ar@cs.msu.ru Subject: Re: [PATCH 1/2] Introduce new completion for Linux task capabilities X-SA-Exim-Version: 4.2.1 X-SA-Exim-Scanned: Yes (on mail.cs.msu.ru) X-Seq: 48208 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: --sOlSqEoF9c4nUQtT Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 active= ly >=20 > 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. >=20 > > > > uses _capability_names. As for _capabilities, >=20 > 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 =E2=80=94 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 =C2=AB_capabilities=C2=BB is still too unclear, I suggest =C2=AB_task_ca= pabilities=C2=BB. >=20 > > > 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, =C2=ABcompadd -P=C2=BB, et al.. > > > > Do you mean invoking =C2=AB_capabilities -O array_name=C2=BB to pass th= e -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. >=20 > No. You would call _capabilities with compadd style options to deal with > the prefixes. You have the following: >=20 > matches=3D( {-,+}"${(@)^caps#cap_}" ) >=20 > 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:=3Dcap_') 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. >=20 > Adding prefixes is easier. If some other function needs capabilities > with a cap_ prefix, that can then call it with -p 'cap_', >=20 > 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 =C2=ABcompset -P [+-]=C2=BB and elimin= ate the need for _capability_names; I'm going to send the revised patch shortly. Thanks a lot for the thorough explanation! >=20 > > > > +if [[ $OSTYPE !=3D linux* ]]; then > > > > + _default; return > > > > +fi > > >=20 > > > 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 = =C2=ABssh > > > $linuxbox foo --with-capability=3D=C2=BB. > > > > 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. >=20 > 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. >=20 > > > Suggest to list these one per line, because: > > I'd personally like them being listed one per line, too. Will do. >=20 > 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. >=20 > Arseny Maslennikov [patch 2/2] wrote: > > +__setpriv_prctl_securebits_set_element() { >=20 > 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_=E2=80=A6 or _git-=E2=80=A6 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 __. >=20 > 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. >=20 > > + bits=3D(noroot noroot_locked > > + no_setuid_fixup no_setuid_fixup_locked >=20 > > + _wanted minus-plus-securebits expl 'prctl securebits' \ > > + compadd "$@" -a - matches >=20 > 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=3D( assignment. >=20 > 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. >=20 > 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. >=20 > > +__setpriv_prctl_securebits_set() { > > + _sequence __setpriv_prctl_securebits_set_element > > +} >=20 > 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. =E2=80=A6_sets So =C2=AB: : _sequence __setpriv_prctl_securebit_set_elements=C2=BB. 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... >=20 > > +__setpriv_caps_all() { > > + # Nonlocal expl; _description call expected. >=20 > Is that necessary? Aren't descriptions passed through with "$@"? I checked with _complete_debug; they indeed are. >=20 > > +__setpriv_cap_set_element() { > > + # We pass through arguments added by _sequence. > > + local -a Oargv=3D( "$@" ) > > + _alternative -O Oargv \ > > + 'special-actions:drop/obtain all caps:__setpriv_caps_all' \ >=20 > 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. >=20 > > + 'minus-plus-caps:capabilities:__setpriv_capability_expressions' \ >=20 > Again, use singular form for the description. >=20 > > +local context state state_descr line > > +typeset -A opt_args >=20 > Given that you've not used states, these are superfluous. >=20 > > + '(- : *)*'{-d,--dump}'[display the current privilege state]' \ >=20 > 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]' =66rom the handler. I'll send that as a separate patch in the series. >=20 > > + '--clear-groups[clear supplementary groups]' \ > > + '--groups[set supplementary groups]:groups:_groups' \ >=20 > 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. >=20 > > + '--inh-caps[set inheritable caps]:capability set: __setpriv_cap_set'= \ > > + '--ambient-caps[set ambient caps]:capability set: __setpriv_cap_set'= \ >=20 > 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. >=20 > > + '--securebits[set "process securebits"]:prctl securebits:__setpriv_p= rctl_securebits_set' \ >=20 > Starting with this one is a run of plural descriptions. --sOlSqEoF9c4nUQtT Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE56JD3UKTLEu/ddrm9dQjyAYL01AFAmBXQfMACgkQ9dQjyAYL 01Al7RAAgFsBc6ylSNT8HkYL1ufIbA6clyasDdAuzuX+qGPezh8yHedeGMpWk+Tq c81pFxX2i2uRtZ8xjsg3F30SBv3jLQWb/rgJqujXzBwv6bpWNUJQi4c/2/GGVIRA +g/0nWDozOAaDkyG5UxH0TwgNMu8yrRGOjLs7mMzid3kkEDJzIumxDdfZ/iC9Pgk R3nRCKejKG11Z5eFJaRpFDFIrBp7UjdqBJuclJyg0KWf0WavDy31g4zsrj6CzksX j2Lra1Vy3EgwJIZbaCzufzw2/LLIm/aFrftxU/PQFGCLvWLK7EXp7741a35LgC4J Ej/mRNvSoLEpTqto0BS9k0E7LJSR7PhK5dOC+3sTf+0/4TDicCVR45NXod4RZxio 0rmTU2AEdrKEiuejSuFeTQglq1jtN8Io8jfE8W1fmB1pa97/Yqr7ZUu/BO0uv71B gM9oTNsXjQsgNCnCPp7MGVa1vCg9if5vYX/2Frb3M6ZTYR6cpBZAlDkbgbCUMfw0 BFfJMu1q+vnObQBQQvV6K/DtrqQ/b6lObb5ByHY6uIw6DRAU4ZwV/tGRv/ea8lnx pC43ICgVDTk9p0O54LVd49poqSZoBZ0pzbMPLqQOwbGRgRAKuPYcAP0X/z7JcJPA IJ8/KMoP5iqb28d3fBzcB8dvz9DG2Q5kKflX8ZQiWj2Lu/CdopY= =eJk0 -----END PGP SIGNATURE----- --sOlSqEoF9c4nUQtT--