PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=199976
Submitted By: Nikolai Lifanov
Details
Details
- Reviewers
bjk eadler mjg lifanov_mail.lifanov.com marcel bapt - Group Reviewers
manpages - Commits
- rS286289: xargs now takes -P0, creating as many concurrent processes as possible
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| usr.bin/xargs/xargs.c | ||
|---|---|---|
| 170 ↗ | (On Diff #5545) | I do not see how nargs is the correct value to use here; it has a different meaning to the program, and can be overridden by other command-line arguments, giving a bizzare behavior change if -n 1 -P 0 are passed in different orders. |
Comment Actions
Good point. The -nX flag overrides nargs depending on the position.
Let me think of a different value to use.
| usr.bin/xargs/xargs.c | ||
|---|---|---|
| 170 ↗ | (On Diff #5545) | Good point. I didn't trip this up by pure coincidence. |
Comment Actions
This replaces the use of nargs, which is arbitrary and can be
overridden by the "-n" flag by value of kern.maxprocs.
This was suggested by Mateusz Guzik on current@.
| usr.bin/xargs/xargs.c | ||
|---|---|---|
| 171 ↗ | (On Diff #5572) | We should probably use the same sysctl instead of nargs for the upper bounds as well |
| usr.bin/xargs/xargs.c | ||
|---|---|---|
| 169 ↗ | (On Diff #5574) | Why allow value larger than RLIMIT_NPROC or _SC_CHILD_MAX? Soft limit would cap number of processes sooner. And kern.maxproc can be larger than "infinity" hard limit. $ limits -Su 50 sh -c "yes | xargs -n1 -P0 yes foo" >/dev/null & $ pgrep -f foo | wc -l 6 $ limits -Su Resource limits (current): maxprocesses-cur 100 $ limits -Hu Resource limits (current): maxprocesses-max 1458 $ sysctl kern.max{proc,users} kern.maxproc: 1620 # (20 + 16 * maxusers) kern.maxusers: 100 |
Comment Actions
I don't understand: don't limits work correctly here if they allow for fewer jobs than maxproc?
Comment Actions
They do, so no need to even check with kern.maxproc for the upper bound. And value less than LONG_MAX can break compatibility for xargs -P<N> in scripts that stress-test system (many processes vs. low memory) - our xargs(1) (unlike GNU) would abort instead of hitting the limits.
| usr.bin/xargs/xargs.c | ||
|---|---|---|
| 51 ↗ | (On Diff #5574) | style(9) doesn't allow both <sys/types.h> and <sys/param.h> included at the same time. |
| 68 ↗ | (On Diff #5574) | Why? Other uses of strtou*() in the file or across the base don't use macros for integer bases. usr.bin/xargs/xargs.c: Rflag = strtol(optarg, &endptr, 10); usr.bin/xargs/xargs.c: Sflag = strtoul(optarg, &endptr, 10); |
| usr.bin/xargs/xargs.c | ||
|---|---|---|
| 68 ↗ | (On Diff #5574) | This was recommended by davide@ |
| usr.bin/xargs/xargs.c | ||
|---|---|---|
| 68 ↗ | (On Diff #5666) | Yeah, it was on IRC: |
| usr.bin/xargs/xargs.c | ||
|---|---|---|
| 175 ↗ | (On Diff #5666) | RLIMIT_NPROC won't mean anything to the end user. |
Comment Actions
Except for one small wording not, the man page changes look okay. Others have spoken for the code. What needs to be done to move this along?
| usr.bin/xargs/xargs.1 | ||
|---|---|---|
| 214 ↗ | (On Diff #5666) | "at a time" is kind of weird at the end of this sentence. It could be removed without changing the meaning of the sentence, or maybe reword for better clarity. |
Comment Actions
o Address feedback from wblock@ by simplifying wording.
For what it's worth, this was the wording similar to GNU info page.
o Replace RLIMIT_NPROC in a user-facing message with a user-friendly
description of what this value is.
| usr.bin/xargs/xargs.1 | ||
|---|---|---|
| 214 ↗ | (On Diff #6188) | Why would a negative value be allowed? gnu xargs does not allow one. This sentence does not mention the upper limit present in the code (and erroring out if not met), which is actually a good thing since the limit should be removed. |
| usr.bin/xargs/xargs.c | ||
| 68 ↗ | (On Diff #6188) | I also don't see what's the point of of this deifne. |
| 116 ↗ | (On Diff #6188) | Why is this set here? |
| 178 ↗ | (On Diff #6188) | The code form user perspective should behave just like gnu xargs, unless there is a good reason to deviate, which I don't see. In this particular case, they have "unlimited" for an argument == 0, specified limit for > 0 and error out for non-numbers and negative values. Here negative values are accepted for some reason. One big not-user-visible difference is the fact that gnu xargs dynamically resizes the process table, while our xargs allocates it upfront. This means we have to have a reasonable limit which can pose as "unlimited" for practical purposes. getrlimit seen here is fine enough. However, the code must not complain about passed limit being bigger. It should internally cap it to getrlimit so that user-visible behaviour is identical with gnu in this respect. As a result the code should explicitly coompare maxprocs to 0 (as opposed to < 1), proclim variable has no use. Fetch rlimit /after/ maxprocs is validated. If (maxprocs == 0 || maxprocs > reslimit.rlim_cur) maxprocs = reslimit.rlim_cur; getrlimit realistically should never fail, so a failure with err(1, "getrlimit"); should be fine enough for surprising cases. |
Comment Actions
Address comments by mjg:
This version tries to get user-facing behavior as close to GNU xargs
as possible. As for why BASE_DECIMAL is defined, I don't know...
The initial patch used a literal 10 here, as seen in other code.
| usr.bin/xargs/xargs.1 | ||
|---|---|---|
| 214 ↗ | (On Diff #6690) | The current text is good, but I wonder if this is better: If |