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=-1.0 required=5.0 tests=MAILING_LIST_MULTI, UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.4 Received: from authenticated user by zero.zsh.org with local id 1ki4S6-000NSH-4i; Wed, 25 Nov 2020 23:43:58 +0000 Received: from authenticated user by zero.zsh.org with esmtpsa (TLS1.3:TLS_AES_256_GCM_SHA384:256) id 1ki4Rj-000NDS-OB; Wed, 25 Nov 2020 23:43:36 +0000 Received: from [192.168.178.21] (helo=hydra) by mail.kiddle.eu with esmtp(Exim 4.93.0.4) (envelope-from ) id 1ki4Rh-000JKq-Tj; Thu, 26 Nov 2020 00:43:34 +0100 In-reply-to: <20201123214942.hi2rx7n3jk25ucmd@chazelas.org> From: Oliver Kiddle References: <20201123214942.hi2rx7n3jk25ucmd@chazelas.org> To: Zsh hackers list , Stephane Chazelas Subject: Re: [PATCHv1] [long] improvements to limit/ulimit API and doc MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <74326.1606347813.1@hydra> Date: Thu, 26 Nov 2020 00:43:33 +0100 Message-ID: <74327-1606347813.918593@HxCz.NV4p.AwzH> X-Seq: 47629 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: Stephane Chazelas wrote: > - I've only tested it on Ubuntu, FreeBSD and Cygwin. I suspect > my test cases will fail on 32bit systems. They're skipped on I tried building and running tests on Solaris 11 and a 32bit build on Solaris 10. I also checked OpenBSD, NetBSD and Dragonfly. There were quite a few other issues but nothing introduced by your patch and your tests passed. Let me know if there's anything additionally that it'd be useful for me to test manually. I've got a few comments, mostly on the documentation. > -If no var(resource) is given, print all limits. > +If tt(-s) is given without other arguments, the resource limits of the > +current shell is set to the previously set resource limits of the s/is/are/ > +sitem(tt(rt_priority))(Maximum realtime priority.) > +sitem(tt(rt_time))(Maximum realtime CPU time slice.) >From the tcsh manual, these appear to be called maxrtprio and maxrttime there. But we already had these names, right? Similarly tcsh has posixlocks. > -var(limit) is a number, with an optional scaling factor, as follows: > +If var(limit) is a decimal integer number without suffix, then for > +historical reason and compatibility with csh where that command comes from, s/reason/reasons/ Or perhaps just drop the first three words of that line. > +the unit will depend on the type of limit: microsecond for tt(rt_time), > +second for all other time limits, kibibytes (1024 bytes) for all limits that "microseconds" and "seconds" - units are mostly always plural. > +The same suffixes are recognised as for tt(limit), but bear in mind that > +default units when no suffix is specified varry between the two. s/varry/vary/ > +performed with kibibytes, mebibytes, or blocks (of 512 bytes; catch: not > +em(pebibytes)) instead. (On some systems additional specifiers are available Not sure about the use of "catch:" there. I'd likely apply emphasis to not instead > +The `limit` builtin accepts more units for resource limits, now shared > +with the `ulimit` builtin allowing to specify arbitrary limit values. "allowing to" isn't correct unless you add an object for the verb "allow" such as "you". It might be better to reword it in the passive tense ("to be specified"). > +`limit -s somelimit` now reports the shell process' own value of the limit. > +The `limit` and `unlimit` builtins are now available again in csh emulation. > +More generally, the resouce limit handling interfaces and documentation have "resource". "More generally" doesn't really add anything useful to the sentence. > +++ b/Src/Builtins/rlimits.c > @@ -55,7 +55,8 @@ typedef struct resinfo_T { > * 1. Add zsh_LIMIT_PRESENT(RLIMIT_XXX) in configure.ac. > * 2. Add an entry for RLIMIT_XXX to known_resources[]. > * Make sure the option letter (resinto_T.opt) is unique. > - * 3. Build zsh and run the test B12rlimit.ztst. > + * 3. Add entry in documentation for the limit and ulimit builtins > + * 4. Build zsh and run the test B12rlimit.ztst. 5. Update Completion/Zsh/Command/_ulimit Units were often fairly well documented there but are missing in a couple of cases so you may be able to add a couple. > 0:check if limit option letters are unique > > + if sh -c 'ulimit 2048' > /dev/null 2>&1; then This doesn't work on Solaris or any of the BSDs except FreeBSD - you have to specify -f to limit file sizes. So it would be skipped despite the fact that the test could otherwise work. Isn't -f fairly standard? >From my testing of the changes, it definitely looks like a nice improvement. Oliver