Re: [PATCHv1] [long] improvements to limit/ulimit API and doc

[Stephane Chazelas <stephane@xxxxxxxxxxxx>]

2020-11-25 00:35:12 +0000, Daniel Shahaf:
[...]
> > A few issues addressed:
> > 
> 
> This patch is over 1k lines long and makes multiple independent changes.
> 
> Could you please split this into one patch per logical change?  That
> would make it easier to review, both now, and in the future should
> a regression be bisected to it.
[...]

Hi Daniel,

thanks for having looking into it.

They're not independent, except maybe for the "limit"/"unlimit"
now back in csh which is a 5 code line change.

Chunks of the code and doc have been rewritten to address
those. Separating out the changes would mean rewriting the code
several times which would be more effort for me and reviewers,
mostly wasted if we're only keeping the last iteration.

But maybe before looking in detail at the code, we can discuss
whether the change in API makes sense.


> > * msgqueue limit specifies a number of bytes but was classified as
> >   ZLIMTYPE_NUMBER which meant k/m suffixes were not allowed and the
> >   default unit for `limit` was 1 (byte) instead of the 1024 (KiB)
> >   specified by documentation.
> > 
> >   -> turns out tcsh's `limit` handled that limit (called `maxmessage`
> >      there) the same way and `bash`, `bosh` and `mksh`'s `ulimit` (not
> >      ksh93 which takes kibibytes there) also expect a number of bytes
> >      there.
> > 
> >      So, the type was changed to ZLIMTYPE_MEMORY, but the unit remains
> >      bytes for both `limit` and `ulimit` as a (now documented) special
> >      quirk for backward compatibility.
> > 
> > * unit suffixes, where not supported (like for ZLIMTYPE_NUMBER) are
> >   silently ignored.
> > 
> >   -> now return an error on unexpected or unrecognised suffixes.
> > 
> > * limit output using ambigous kB, MB units. These days, those tend to
> >   imply decimal scaling factors (1000B, 1000000B).
> > 
> >   -> changed output to use KiB, MiB, the ISO 80000 unambiguous ones
> >      which are now more or less mainstream.
> >   -> those, extended to KMGTPE..iB are also accepted on input while
> >      KB, MB... are interpreted as decimal. (K, M, G remain binary).
> >   -> documentation updated to avoid kilobyte, megabyte and use
> >      unambiguous kibibyte, mebibyte... instead.
> > 
> > -> rt_time limit is now `ulimit -R` like in bash or bosh.
> > 
> > * "nice" limit description ("max nice") was misleading as it's more
> >   linked to the *minimum* niceness the process can get.
> > 
> >   -> Changed to "max nice priority" matching the Linux kernel's own
> >      description.
> > 
> > * documentation for both `limit` and `ulimit` missing a few limits ->
> >   added
> > 
> > * time limits are output as h:mm:ss but that's not accepted on input.
> >   1:00:00 silently truncated to 1:00 (one minute instead of one hour).
> > 
> >   -> accept [[hh:]mm:]ss[.usec]
> > 
> > * limit and ulimit output truncate precision (1000B limit represented as
> >   0kB and 1 respectively)
> > 
> >   -> addressed in `limit` but not `ulimit` (where
> >      compliance/compatibility with ksh matters). By only using scaling
> >      factors when the limit can be represented in an integer number of
> >      them (using B, as in 1000B above when none qualify).
> > 
> > * some limits can't be set to arbitrary values (like that 1000 above for
> >   filesize) and it's not always obvious what the default unit should be.
> > 
> >   -> recognise the B suffix on input for "memory" type limits and
> >      "s"/"ms"/"ns" for time/microsecond type units so the user can input
> >      values unambiguously.
> >   -> those suffixes are now recognised by both limit and ulimit. Parsing
> >      code factorised into `zstrtorlimt`.
> > 
> > * `limit` and `unlimit` are disabled when zsh is started in csh
> >   emulation even though those builtins come from there.
> > 
> >   -> changed the features_emu in rlimits.mdd (only used there) and
> >     mkbltnmlst.sh to features_posix for those to only be disabled
> >     in sh/ksh emulation.
> > 
> > * `limit` changes the limits for children, `ulimit` for the shell,
> >   but both report limits for children, and it's not possible to
> >   retrieve the shell's own limits.
> > 
> >   -> ulimit not changed as it's probably better that way.
> >   -> `limit -s somelimit` changed so it reports the somelimit value
> >      for the shell process
> > 
> > -> documentation improved.
> > ---
> >  Doc/Zsh/builtins.yo      | 129 +++++++++----
> >  Doc/Zsh/expn.yo          |  18 +-
> >  Doc/Zsh/mod_zpty.yo      |   4 +-
> >  Doc/Zsh/params.yo        |  10 +-
> >  NEWS                     |   7 +
> >  Src/Builtins/rlimits.c   | 390 +++++++++++++++++++++++++++------------
> >  Src/Builtins/rlimits.mdd |   8 +-
> >  Src/mkbltnmlst.sh        |  10 +-
> >  Test/B12limit.ztst       | 129 +++++++++++++
> >  9 files changed, 530 insertions(+), 175 deletions(-)
> > 
> > Though I think it could be applied as is (I don't think I've
> > broken backward compatibility), there are a few things I'm not
> > completely happy or sure about so I'd appreciate some input.
> > 
> > The few remaining issues I've not really addressed here:
> > 
> > - ulimit output rounds down the values (some of them anyway) so
> >   we lose information. Is it worth addressing? (like with a
> >   "raw" option)?
> > 
> > - Some might disapprove the switch to kibibyte/mebibyte KiK/MiB
> >   (and the MB meaning 1,000,000).
> > 
> > - Is it worth accepting floating point values like:
> >   limit filesize 1.2G
> > 
> > - I've only tested it on Ubuntu, FreeBSD and Cygwin. I suspect
> >   my test cases will fail on 32bit systems. They're skipped on
> >   Cygwin which doesn't let you set any limit AFAICT. I don't
> >   have much coverage in those two tests, but with limits being
> >   very system specific, I'm not sure how to tackle it.
> > 
> > - ulimit still outputs the limits set for children processes. I
> >   think that's probably best. So it outputs the limits set by
> >   limit, ulimit or ulimit -s, even if strictly speaking, it
> >   doesn't give you what's returned by getrlimit() like in other
> >   shells (that only ever becomes visible if you use "limit"
> >   anyway which is not in those other shells.
> > 
> > - there are some corner case issues that could surprise users
> >   and may be worth documenting like:
> >   (limit filesize 1k; (print {1..2000} > file)) still creates a
> >   8K file because the fork for the (print...) was optimised out
> >   so the limits are not applied.
> > 
> > - I've made it so "limit -s filesize" reports the shell's own
> >   limit (-s is for "set" initially, but it could also be "shell"
> >   or "self"). But while "limit" outputs all children limits,
> >   "limit -s" still copies those children limits to the shell's
> >   and there's no way to print *all* self limits. That doesn't
> >   make for a very intuitive API.
>