Page MenuHomeFreeBSD
Differential D36407

ls: `-v` flag
ClosedPublic
Actions

Authored by obiwac_gmail.com on Aug 31 2022, 7:48 PM.
Tags
None
Referenced Files
F114440094: D36407.id110356.diff
Sat, Apr 12, 2:04 PM
Unknown Object (File)
Fri, Apr 11, 9:44 AM
Unknown Object (File)
Fri, Apr 11, 8:09 AM
Unknown Object (File)
Mon, Apr 7, 5:22 AM
Unknown Object (File)
Sun, Apr 6, 1:10 PM
Unknown Object (File)
Feb 21 2025, 8:18 PM
Unknown Object (File)
Feb 21 2025, 5:19 PM
Unknown Object (File)
Feb 10 2025, 9:53 PM
View All Files
Subscribers
bcr
imp

Details

Reviewers
pstef
pauamma_gundo.com
Group Reviewers
manpages
Commits
rG36802990d64c: ls(1): add a -v flag to sort naturally
rGe2662256cdbc: ls(1): add a -v flag to sort naturally
Summary

Add a -v flag for ls(1) which sorts entries following a natural ordering using strverscmp(3) (e.g. "bloem1 bloem9 bloem10" as opposed to "bloem1 bloem10 bloem9").

Update manual page for ls(1).

Add test for ls -v.

Test Plan

Tested through the tests I wrote.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 47293
Build 44180: arc lint + arc unit

Event Timeline

obiwac_gmail.com created this revision.Aug 31 2022, 7:48 PM
Herald added a subscriber: imp. · View Herald TranscriptAug 31 2022, 7:48 PM
obiwac_gmail.com requested review of this revision.Aug 31 2022, 7:48 PM
Harbormaster completed remote builds in B47208: Diff 110023.Aug 31 2022, 7:48 PM
obiwac_gmail.com updated this revision to Diff 110024.Aug 31 2022, 7:51 PM

ls: Add strcoll(3) & strverscmp(3) to "SEE ALSO" in manpage

Harbormaster completed remote builds in B47209: Diff 110024.Aug 31 2022, 7:52 PM
obiwac_gmail.com added inline comments.Aug 31 2022, 11:05 PM
bin/ls/ls.1
893

I forgot a comma here when reordering.

obiwac_gmail.com updated this revision to Diff 110040.EditedSep 1 2022, 11:33 AM

Fix commas in ls.1

Harbormaster completed remote builds in B47217: Diff 110040.Sep 1 2022, 11:33 AM
pauamma_gundo.com added a reviewer: manpages.Sep 6 2022, 8:13 PM
bcr added a subscriber: bcr.Sep 7 2022, 9:46 AM

One more comment about the manpage. Did you run "mandoc -Tlint" over it?

bin/ls/ls.1
407

This needs a line break after the sentence stop.

obiwac_gmail.com added a comment.Sep 7 2022, 7:00 PM

I have not, I didn't know about that, thanks for pointing me to it! I'll fix that in a bit. May I fix all the "STYLE: useless macro: Tn" errors or do I ignore those?

bcr added a comment.Sep 8 2022, 9:13 AM
In D36407#828577, @obiwac_gmail.com wrote:

I have not, I didn't know about that, thanks for pointing me to it! I'll fix that in a bit. May I fix all the "STYLE: useless macro: Tn" errors or do I ignore those?

I suggest you do this in a separate commit to not bloat this one too much.
That way, we can separate individual changes easier and the review time in also reduced as people can focus on a single change.

obiwac_gmail.com updated this revision to Diff 110352.Sep 8 2022, 11:48 AM

Fix warnings from mandoc -Tlint

Harbormaster completed remote builds in B47293: Diff 110352.Sep 8 2022, 11:48 AM
obiwac_gmail.com edited the summary of this revision. (Show Details)Sep 8 2022, 11:48 AM
obiwac_gmail.com updated this revision to Diff 110356.Sep 8 2022, 3:31 PM

ls: Remove Tn macros from manpage

Harbormaster completed remote builds in B47294: Diff 110356.Sep 8 2022, 3:31 PM
bcr accepted this revision as: manpages.Oct 12 2022, 4:00 PM

Manpage looks good now. Thank you!

pauamma_gundo.com accepted this revision.Oct 12 2022, 5:32 PM

Both fixable on commit.

bin/ls/ls.1
35

Bump on commit.

403
This revision is now accepted and ready to land.Oct 12 2022, 5:32 PM
Closed by commit rGe2662256cdbc: ls(1): add a -v flag to sort naturally (authored by obiwac_gmail.com, committed by pstef). · Explain WhyOct 30 2022, 11:14 PM
This revision was automatically updated to reflect the committed changes.
pstef added a commit: rGe2662256cdbc: ls(1): add a -v flag to sort naturally.
pstef added a commit: rG36802990d64c: ls(1): add a -v flag to sort naturally.Dec 24 2022, 7:22 AM

Revision Contents
Changeset List

PathSize
bin/
ls/
14 lines
2 lines
22 lines
22 lines
tests/
15 lines
4 lines

Diff 110352

View Options

bin/ls/cmp.c

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Lines
static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93"; static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93";
#endif /* not lint */ #endif /* not lint */
#endif #endif
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include <sys/param.h> #include <sys/param.h>
#include <sys/wait.h> #include <sys/wait.h>
#include <sys/types.h>
jbeichUnsubmitted
Done
Inline Actions

style(9) doesn't allow both <sys/types.h> and <sys/param.h> included at the same time.

jbeich: style(9) doesn't allow both `<sys/types.h>` and `<sys/param.h>` included at the same time.
mjgUnsubmitted
Not Done
Inline Actions

sys/types.h must be included prior to sys/wait.h.

mjg: sys/types.h must be included prior to sys/wait.h.
#include <sys/sysctl.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h> #include <fcntl.h>
#include <langinfo.h> #include <langinfo.h>
#include <locale.h> #include <locale.h>
#include <paths.h> #include <paths.h>
#include <regex.h> #include <regex.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include "pathnames.h" #include "pathnames.h"
#define BASE_DECIMAL 10
jbeichUnsubmitted
Not Done
Inline Actions

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);

http://bxr.su/search?refs=strtou*&project=FreeBSD

jbeich: Why? Other uses of `strtou*()` in the file or across the base don't use macros for integer…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

This was recommended by davide@
I can change all of the references, or remove the macro.

allanjude: This was recommended by davide@ I can change all of the references, or remove the macro.
jbeichUnsubmitted
Not Done
Inline Actions

@davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback here, in the bug or on freebsd-current maillist.

jbeich: @davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

Yeah, it was on IRC:
<davide> AllanJude: you don't do upper bound validation on the number of processes
<davide> AllanJude: '10' is hardcoded and makes hard to understand what that's actually about
<AllanJude> the 10 is the 'base'
<AllanJude> it is a standard argument to strtol
<davide> AllanJude: I can read the manpage
<davide> AllanJude: it should be a macro
<AllanJude> there is a macro for decimal?
<davide> #define BLAH 10 ?
<AllanJude> so, I should make one?
<davide> otherwise, lgtm

allanjude: Yeah, it was on IRC: <davide> AllanJude: you don't do upper bound validation on the number of…
mjgUnsubmitted
Not Done
Inline Actions

I also don't see what's the point of of this deifne.

mjg: I also don't see what's the point of of this deifne.
static void parse_input(int, char *[]); static void parse_input(int, char *[]);
static void prerun(int, char *[]); static void prerun(int, char *[]);
static int prompt(void); static int prompt(void);
static void run(char **); static void run(char **);
static void usage(void); static void usage(void);
void strnsubst(char **, const char *, const char *, size_t); void strnsubst(char **, const char *, const char *, size_t);
static pid_t xwait(int block, int *status); static pid_t xwait(int block, int *status);
static void xexit(const char *, const int); static void xexit(const char *, const int);
Show All 21 Lines
extern char **environ; extern char **environ;
int int
main(int argc, char *argv[]) main(int argc, char *argv[])
{ {
long arg_max; long arg_max;
int ch, Jflag, nargs, nflag, nline; int ch, Jflag, nargs, nflag, nline;
size_t linelen; size_t linelen;
size_t proclen;
mjgUnsubmitted
Not Done
Inline Actions

I would rename this to rl or rlimit, as a nit.

mjg: I would rename this to rl or rlimit, as a nit.
int proclim;
char *endptr; char *endptr;
inpline = replstr = NULL; inpline = replstr = NULL;
ep = environ; ep = environ;
eofstr = ""; eofstr = "";
Jflag = nflag = 0; Jflag = nflag = 0;
mjgUnsubmitted
Not Done
Inline Actions

Why is this set here?

mjg: Why is this set here?
(void)setlocale(LC_ALL, ""); (void)setlocale(LC_ALL, "");
/* /*
* POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that * POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that
* caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given * caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given
* that the smallest argument is 2 bytes in length, this means that * that the smallest argument is 2 bytes in length, this means that
* the number of arguments is limited to: * the number of arguments is limited to:
* *
Show All 36 Lines case 'n':
nflag = 1; nflag = 1;
if ((nargs = atoi(optarg)) <= 0) if ((nargs = atoi(optarg)) <= 0)
errx(1, "illegal argument count"); errx(1, "illegal argument count");
break; break;
case 'o': case 'o':
oflag = 1; oflag = 1;
break; break;
case 'P': case 'P':
if ((maxprocs = atoi(optarg)) <= 0) proclen = sizeof(proclim);
errx(1, "max. processes must be >0"); sysctlbyname("kern.maxproc", &proclim, &proclen, NULL, 0);
jbeichUnsubmitted
Done
Inline Actions

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
jbeich: Why allow value larger than `RLIMIT_NPROC` or `_SC_CHILD_MAX`? Soft limit would cap number of…
maxprocs = strtol(optarg, &endptr, BASE_DECIMAL);
if (*endptr != '\0')
errx(1, "max processes must be a number");
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

We should probably use the same sysctl instead of nargs for the upper bounds as well

allanjude: We should probably use the same sysctl instead of nargs for the upper bounds as well
if (maxprocs >= proclim)
errx(1, "max processes must be less than kern.maxproc: %d", proclim);
bjkUnsubmitted
Done
Inline Actions

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.

bjk: I do not see how nargs is the correct value to use here; it has a different meaning to the…
lifanov_mail.lifanov.comUnsubmitted
Done
Inline Actions

Good point. I didn't trip this up by pure coincidence.
Let me think of a different value to use.

lifanov_mail.lifanov.com: Good point. I didn't trip this up by pure coincidence. Let me think of a different value to use.
lifanov_mail.lifanov.comUnsubmitted
Not Done
Inline Actions

RLIMIT_NPROC won't mean anything to the end user.
How about changing the line to just:
errx(1, "max processes must be less than %d", proclim);?

lifanov_mail.lifanov.com: RLIMIT_NPROC won't mean anything to the end user. How about changing the line to just: errx(1…
if (maxprocs < 1)
maxprocs = proclim;
break; break;
mjgUnsubmitted
Not Done
Inline Actions

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.

mjg: The code form user perspective should behave just like gnu xargs, unless there is a good reason…
case 'p': case 'p':
pflag = 1; pflag = 1;
break; break;
case 'R': case 'R':
Rflag = strtol(optarg, &endptr, 10); Rflag = strtol(optarg, &endptr, 10);
if (*endptr != '\0') if (*endptr != '\0')
errx(1, "replacements must be a number"); errx(1, "replacements must be a number");
break; break;
▲ Show 20 Lines • Show All 600 Lines • Show Last 20 Lines
View Options

bin/ls/extern.h

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Lines
static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93"; static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93";
#endif /* not lint */ #endif /* not lint */
#endif #endif
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include <sys/param.h> #include <sys/param.h>
#include <sys/wait.h> #include <sys/wait.h>
#include <sys/types.h>
jbeichUnsubmitted
Done
Inline Actions

style(9) doesn't allow both <sys/types.h> and <sys/param.h> included at the same time.

jbeich: style(9) doesn't allow both `<sys/types.h>` and `<sys/param.h>` included at the same time.
mjgUnsubmitted
Not Done
Inline Actions

sys/types.h must be included prior to sys/wait.h.

mjg: sys/types.h must be included prior to sys/wait.h.
#include <sys/sysctl.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h> #include <fcntl.h>
#include <langinfo.h> #include <langinfo.h>
#include <locale.h> #include <locale.h>
#include <paths.h> #include <paths.h>
#include <regex.h> #include <regex.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include "pathnames.h" #include "pathnames.h"
#define BASE_DECIMAL 10
jbeichUnsubmitted
Not Done
Inline Actions

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);

http://bxr.su/search?refs=strtou*&project=FreeBSD

jbeich: Why? Other uses of `strtou*()` in the file or across the base don't use macros for integer…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

This was recommended by davide@
I can change all of the references, or remove the macro.

allanjude: This was recommended by davide@ I can change all of the references, or remove the macro.
jbeichUnsubmitted
Not Done
Inline Actions

@davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback here, in the bug or on freebsd-current maillist.

jbeich: @davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

Yeah, it was on IRC:
<davide> AllanJude: you don't do upper bound validation on the number of processes
<davide> AllanJude: '10' is hardcoded and makes hard to understand what that's actually about
<AllanJude> the 10 is the 'base'
<AllanJude> it is a standard argument to strtol
<davide> AllanJude: I can read the manpage
<davide> AllanJude: it should be a macro
<AllanJude> there is a macro for decimal?
<davide> #define BLAH 10 ?
<AllanJude> so, I should make one?
<davide> otherwise, lgtm

allanjude: Yeah, it was on IRC: <davide> AllanJude: you don't do upper bound validation on the number of…
mjgUnsubmitted
Not Done
Inline Actions

I also don't see what's the point of of this deifne.

mjg: I also don't see what's the point of of this deifne.
static void parse_input(int, char *[]); static void parse_input(int, char *[]);
static void prerun(int, char *[]); static void prerun(int, char *[]);
static int prompt(void); static int prompt(void);
static void run(char **); static void run(char **);
static void usage(void); static void usage(void);
void strnsubst(char **, const char *, const char *, size_t); void strnsubst(char **, const char *, const char *, size_t);
static pid_t xwait(int block, int *status); static pid_t xwait(int block, int *status);
static void xexit(const char *, const int); static void xexit(const char *, const int);
Show All 21 Lines
extern char **environ; extern char **environ;
int int
main(int argc, char *argv[]) main(int argc, char *argv[])
{ {
long arg_max; long arg_max;
int ch, Jflag, nargs, nflag, nline; int ch, Jflag, nargs, nflag, nline;
size_t linelen; size_t linelen;
size_t proclen;
mjgUnsubmitted
Not Done
Inline Actions

I would rename this to rl or rlimit, as a nit.

mjg: I would rename this to rl or rlimit, as a nit.
int proclim;
char *endptr; char *endptr;
inpline = replstr = NULL; inpline = replstr = NULL;
ep = environ; ep = environ;
eofstr = ""; eofstr = "";
Jflag = nflag = 0; Jflag = nflag = 0;
mjgUnsubmitted
Not Done
Inline Actions

Why is this set here?

mjg: Why is this set here?
(void)setlocale(LC_ALL, ""); (void)setlocale(LC_ALL, "");
/* /*
* POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that * POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that
* caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given * caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given
* that the smallest argument is 2 bytes in length, this means that * that the smallest argument is 2 bytes in length, this means that
* the number of arguments is limited to: * the number of arguments is limited to:
* *
Show All 36 Lines case 'n':
nflag = 1; nflag = 1;
if ((nargs = atoi(optarg)) <= 0) if ((nargs = atoi(optarg)) <= 0)
errx(1, "illegal argument count"); errx(1, "illegal argument count");
break; break;
case 'o': case 'o':
oflag = 1; oflag = 1;
break; break;
case 'P': case 'P':
if ((maxprocs = atoi(optarg)) <= 0) proclen = sizeof(proclim);
errx(1, "max. processes must be >0"); sysctlbyname("kern.maxproc", &proclim, &proclen, NULL, 0);
jbeichUnsubmitted
Done
Inline Actions

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
jbeich: Why allow value larger than `RLIMIT_NPROC` or `_SC_CHILD_MAX`? Soft limit would cap number of…
maxprocs = strtol(optarg, &endptr, BASE_DECIMAL);
if (*endptr != '\0')
errx(1, "max processes must be a number");
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

We should probably use the same sysctl instead of nargs for the upper bounds as well

allanjude: We should probably use the same sysctl instead of nargs for the upper bounds as well
if (maxprocs >= proclim)
errx(1, "max processes must be less than kern.maxproc: %d", proclim);
bjkUnsubmitted
Done
Inline Actions

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.

bjk: I do not see how nargs is the correct value to use here; it has a different meaning to the…
lifanov_mail.lifanov.comUnsubmitted
Done
Inline Actions

Good point. I didn't trip this up by pure coincidence.
Let me think of a different value to use.

lifanov_mail.lifanov.com: Good point. I didn't trip this up by pure coincidence. Let me think of a different value to use.
lifanov_mail.lifanov.comUnsubmitted
Not Done
Inline Actions

RLIMIT_NPROC won't mean anything to the end user.
How about changing the line to just:
errx(1, "max processes must be less than %d", proclim);?

lifanov_mail.lifanov.com: RLIMIT_NPROC won't mean anything to the end user. How about changing the line to just: errx(1…
if (maxprocs < 1)
maxprocs = proclim;
break; break;
mjgUnsubmitted
Not Done
Inline Actions

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.

mjg: The code form user perspective should behave just like gnu xargs, unless there is a good reason…
case 'p': case 'p':
pflag = 1; pflag = 1;
break; break;
case 'R': case 'R':
Rflag = strtol(optarg, &endptr, 10); Rflag = strtol(optarg, &endptr, 10);
if (*endptr != '\0') if (*endptr != '\0')
errx(1, "replacements must be a number"); errx(1, "replacements must be a number");
break; break;
▲ Show 20 Lines • Show All 600 Lines • Show Last 20 Lines
View Options

bin/ls/ls.1

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Lines
static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93"; static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93";
#endif /* not lint */ #endif /* not lint */
#endif #endif
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include <sys/param.h> #include <sys/param.h>
#include <sys/wait.h> #include <sys/wait.h>
#include <sys/types.h>
jbeichUnsubmitted
Done
Inline Actions

style(9) doesn't allow both <sys/types.h> and <sys/param.h> included at the same time.

jbeich: style(9) doesn't allow both `<sys/types.h>` and `<sys/param.h>` included at the same time.
mjgUnsubmitted
Not Done
Inline Actions

sys/types.h must be included prior to sys/wait.h.

mjg: sys/types.h must be included prior to sys/wait.h.
#include <sys/sysctl.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h> #include <fcntl.h>
#include <langinfo.h> #include <langinfo.h>
#include <locale.h> #include <locale.h>
#include <paths.h> #include <paths.h>
#include <regex.h> #include <regex.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include "pathnames.h" #include "pathnames.h"
#define BASE_DECIMAL 10
jbeichUnsubmitted
Not Done
Inline Actions

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);

http://bxr.su/search?refs=strtou*&project=FreeBSD

jbeich: Why? Other uses of `strtou*()` in the file or across the base don't use macros for integer…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

This was recommended by davide@
I can change all of the references, or remove the macro.

allanjude: This was recommended by davide@ I can change all of the references, or remove the macro.
jbeichUnsubmitted
Not Done
Inline Actions

@davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback here, in the bug or on freebsd-current maillist.

jbeich: @davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

Yeah, it was on IRC:
<davide> AllanJude: you don't do upper bound validation on the number of processes
<davide> AllanJude: '10' is hardcoded and makes hard to understand what that's actually about
<AllanJude> the 10 is the 'base'
<AllanJude> it is a standard argument to strtol
<davide> AllanJude: I can read the manpage
<davide> AllanJude: it should be a macro
<AllanJude> there is a macro for decimal?
<davide> #define BLAH 10 ?
<AllanJude> so, I should make one?
<davide> otherwise, lgtm

allanjude: Yeah, it was on IRC: <davide> AllanJude: you don't do upper bound validation on the number of…
mjgUnsubmitted
Not Done
Inline Actions

I also don't see what's the point of of this deifne.

mjg: I also don't see what's the point of of this deifne.
static void parse_input(int, char *[]); static void parse_input(int, char *[]);
static void prerun(int, char *[]); static void prerun(int, char *[]);
static int prompt(void); static int prompt(void);
static void run(char **); static void run(char **);
static void usage(void); static void usage(void);
void strnsubst(char **, const char *, const char *, size_t); void strnsubst(char **, const char *, const char *, size_t);
static pid_t xwait(int block, int *status); static pid_t xwait(int block, int *status);
static void xexit(const char *, const int); static void xexit(const char *, const int);
Show All 21 Lines
extern char **environ; extern char **environ;
int int
main(int argc, char *argv[]) main(int argc, char *argv[])
{ {
long arg_max; long arg_max;
int ch, Jflag, nargs, nflag, nline; int ch, Jflag, nargs, nflag, nline;
size_t linelen; size_t linelen;
size_t proclen;
mjgUnsubmitted
Not Done
Inline Actions

I would rename this to rl or rlimit, as a nit.

mjg: I would rename this to rl or rlimit, as a nit.
int proclim;
char *endptr; char *endptr;
inpline = replstr = NULL; inpline = replstr = NULL;
ep = environ; ep = environ;
eofstr = ""; eofstr = "";
Jflag = nflag = 0; Jflag = nflag = 0;
mjgUnsubmitted
Not Done
Inline Actions

Why is this set here?

mjg: Why is this set here?
(void)setlocale(LC_ALL, ""); (void)setlocale(LC_ALL, "");
/* /*
* POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that * POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that
* caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given * caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given
* that the smallest argument is 2 bytes in length, this means that * that the smallest argument is 2 bytes in length, this means that
* the number of arguments is limited to: * the number of arguments is limited to:
* *
Show All 36 Lines case 'n':
nflag = 1; nflag = 1;
if ((nargs = atoi(optarg)) <= 0) if ((nargs = atoi(optarg)) <= 0)
errx(1, "illegal argument count"); errx(1, "illegal argument count");
break; break;
case 'o': case 'o':
oflag = 1; oflag = 1;
break; break;
case 'P': case 'P':
if ((maxprocs = atoi(optarg)) <= 0) proclen = sizeof(proclim);
errx(1, "max. processes must be >0"); sysctlbyname("kern.maxproc", &proclim, &proclen, NULL, 0);
jbeichUnsubmitted
Done
Inline Actions

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
jbeich: Why allow value larger than `RLIMIT_NPROC` or `_SC_CHILD_MAX`? Soft limit would cap number of…
maxprocs = strtol(optarg, &endptr, BASE_DECIMAL);
if (*endptr != '\0')
errx(1, "max processes must be a number");
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

We should probably use the same sysctl instead of nargs for the upper bounds as well

allanjude: We should probably use the same sysctl instead of nargs for the upper bounds as well
if (maxprocs >= proclim)
errx(1, "max processes must be less than kern.maxproc: %d", proclim);
bjkUnsubmitted
Done
Inline Actions

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.

bjk: I do not see how nargs is the correct value to use here; it has a different meaning to the…
lifanov_mail.lifanov.comUnsubmitted
Done
Inline Actions

Good point. I didn't trip this up by pure coincidence.
Let me think of a different value to use.

lifanov_mail.lifanov.com: Good point. I didn't trip this up by pure coincidence. Let me think of a different value to use.
lifanov_mail.lifanov.comUnsubmitted
Not Done
Inline Actions

RLIMIT_NPROC won't mean anything to the end user.
How about changing the line to just:
errx(1, "max processes must be less than %d", proclim);?

lifanov_mail.lifanov.com: RLIMIT_NPROC won't mean anything to the end user. How about changing the line to just: errx(1…
if (maxprocs < 1)
maxprocs = proclim;
break; break;
mjgUnsubmitted
Not Done
Inline Actions

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.

mjg: The code form user perspective should behave just like gnu xargs, unless there is a good reason…
case 'p': case 'p':
pflag = 1; pflag = 1;
break; break;
case 'R': case 'R':
Rflag = strtol(optarg, &endptr, 10); Rflag = strtol(optarg, &endptr, 10);
if (*endptr != '\0') if (*endptr != '\0')
errx(1, "replacements must be a number"); errx(1, "replacements must be a number");
break; break;
▲ Show 20 Lines • Show All 600 Lines • Show Last 20 Lines
View Options

bin/ls/ls.c

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Lines
static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93"; static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93";
#endif /* not lint */ #endif /* not lint */
#endif #endif
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include <sys/param.h> #include <sys/param.h>
#include <sys/wait.h> #include <sys/wait.h>
#include <sys/types.h>
jbeichUnsubmitted
Done
Inline Actions

style(9) doesn't allow both <sys/types.h> and <sys/param.h> included at the same time.

jbeich: style(9) doesn't allow both `<sys/types.h>` and `<sys/param.h>` included at the same time.
mjgUnsubmitted
Not Done
Inline Actions

sys/types.h must be included prior to sys/wait.h.

mjg: sys/types.h must be included prior to sys/wait.h.
#include <sys/sysctl.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h> #include <fcntl.h>
#include <langinfo.h> #include <langinfo.h>
#include <locale.h> #include <locale.h>
#include <paths.h> #include <paths.h>
#include <regex.h> #include <regex.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include "pathnames.h" #include "pathnames.h"
#define BASE_DECIMAL 10
jbeichUnsubmitted
Not Done
Inline Actions

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);

http://bxr.su/search?refs=strtou*&project=FreeBSD

jbeich: Why? Other uses of `strtou*()` in the file or across the base don't use macros for integer…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

This was recommended by davide@
I can change all of the references, or remove the macro.

allanjude: This was recommended by davide@ I can change all of the references, or remove the macro.
jbeichUnsubmitted
Not Done
Inline Actions

@davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback here, in the bug or on freebsd-current maillist.

jbeich: @davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

Yeah, it was on IRC:
<davide> AllanJude: you don't do upper bound validation on the number of processes
<davide> AllanJude: '10' is hardcoded and makes hard to understand what that's actually about
<AllanJude> the 10 is the 'base'
<AllanJude> it is a standard argument to strtol
<davide> AllanJude: I can read the manpage
<davide> AllanJude: it should be a macro
<AllanJude> there is a macro for decimal?
<davide> #define BLAH 10 ?
<AllanJude> so, I should make one?
<davide> otherwise, lgtm

allanjude: Yeah, it was on IRC: <davide> AllanJude: you don't do upper bound validation on the number of…
mjgUnsubmitted
Not Done
Inline Actions

I also don't see what's the point of of this deifne.

mjg: I also don't see what's the point of of this deifne.
static void parse_input(int, char *[]); static void parse_input(int, char *[]);
static void prerun(int, char *[]); static void prerun(int, char *[]);
static int prompt(void); static int prompt(void);
static void run(char **); static void run(char **);
static void usage(void); static void usage(void);
void strnsubst(char **, const char *, const char *, size_t); void strnsubst(char **, const char *, const char *, size_t);
static pid_t xwait(int block, int *status); static pid_t xwait(int block, int *status);
static void xexit(const char *, const int); static void xexit(const char *, const int);
Show All 21 Lines
extern char **environ; extern char **environ;
int int
main(int argc, char *argv[]) main(int argc, char *argv[])
{ {
long arg_max; long arg_max;
int ch, Jflag, nargs, nflag, nline; int ch, Jflag, nargs, nflag, nline;
size_t linelen; size_t linelen;
size_t proclen;
mjgUnsubmitted
Not Done
Inline Actions

I would rename this to rl or rlimit, as a nit.

mjg: I would rename this to rl or rlimit, as a nit.
int proclim;
char *endptr; char *endptr;
inpline = replstr = NULL; inpline = replstr = NULL;
ep = environ; ep = environ;
eofstr = ""; eofstr = "";
Jflag = nflag = 0; Jflag = nflag = 0;
mjgUnsubmitted
Not Done
Inline Actions

Why is this set here?

mjg: Why is this set here?
(void)setlocale(LC_ALL, ""); (void)setlocale(LC_ALL, "");
/* /*
* POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that * POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that
* caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given * caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given
* that the smallest argument is 2 bytes in length, this means that * that the smallest argument is 2 bytes in length, this means that
* the number of arguments is limited to: * the number of arguments is limited to:
* *
Show All 36 Lines case 'n':
nflag = 1; nflag = 1;
if ((nargs = atoi(optarg)) <= 0) if ((nargs = atoi(optarg)) <= 0)
errx(1, "illegal argument count"); errx(1, "illegal argument count");
break; break;
case 'o': case 'o':
oflag = 1; oflag = 1;
break; break;
case 'P': case 'P':
if ((maxprocs = atoi(optarg)) <= 0) proclen = sizeof(proclim);
errx(1, "max. processes must be >0"); sysctlbyname("kern.maxproc", &proclim, &proclen, NULL, 0);
jbeichUnsubmitted
Done
Inline Actions

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
jbeich: Why allow value larger than `RLIMIT_NPROC` or `_SC_CHILD_MAX`? Soft limit would cap number of…
maxprocs = strtol(optarg, &endptr, BASE_DECIMAL);
if (*endptr != '\0')
errx(1, "max processes must be a number");
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

We should probably use the same sysctl instead of nargs for the upper bounds as well

allanjude: We should probably use the same sysctl instead of nargs for the upper bounds as well
if (maxprocs >= proclim)
errx(1, "max processes must be less than kern.maxproc: %d", proclim);
bjkUnsubmitted
Done
Inline Actions

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.

bjk: I do not see how nargs is the correct value to use here; it has a different meaning to the…
lifanov_mail.lifanov.comUnsubmitted
Done
Inline Actions

Good point. I didn't trip this up by pure coincidence.
Let me think of a different value to use.

lifanov_mail.lifanov.com: Good point. I didn't trip this up by pure coincidence. Let me think of a different value to use.
lifanov_mail.lifanov.comUnsubmitted
Not Done
Inline Actions

RLIMIT_NPROC won't mean anything to the end user.
How about changing the line to just:
errx(1, "max processes must be less than %d", proclim);?

lifanov_mail.lifanov.com: RLIMIT_NPROC won't mean anything to the end user. How about changing the line to just: errx(1…
if (maxprocs < 1)
maxprocs = proclim;
break; break;
mjgUnsubmitted
Not Done
Inline Actions

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.

mjg: The code form user perspective should behave just like gnu xargs, unless there is a good reason…
case 'p': case 'p':
pflag = 1; pflag = 1;
break; break;
case 'R': case 'R':
Rflag = strtol(optarg, &endptr, 10); Rflag = strtol(optarg, &endptr, 10);
if (*endptr != '\0') if (*endptr != '\0')
errx(1, "replacements must be a number"); errx(1, "replacements must be a number");
break; break;
▲ Show 20 Lines • Show All 600 Lines • Show Last 20 Lines
View Options

bin/ls/tests/ls_tests.sh

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Lines
static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93"; static char sccsid[] = "@(#)xargs.c 8.1 (Berkeley) 6/6/93";
#endif /* not lint */ #endif /* not lint */
#endif #endif
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include <sys/param.h> #include <sys/param.h>
#include <sys/wait.h> #include <sys/wait.h>
#include <sys/types.h>
jbeichUnsubmitted
Done
Inline Actions

style(9) doesn't allow both <sys/types.h> and <sys/param.h> included at the same time.

jbeich: style(9) doesn't allow both `<sys/types.h>` and `<sys/param.h>` included at the same time.
mjgUnsubmitted
Not Done
Inline Actions

sys/types.h must be included prior to sys/wait.h.

mjg: sys/types.h must be included prior to sys/wait.h.
#include <sys/sysctl.h>
#include <err.h> #include <err.h>
#include <errno.h> #include <errno.h>
#include <fcntl.h> #include <fcntl.h>
#include <langinfo.h> #include <langinfo.h>
#include <locale.h> #include <locale.h>
#include <paths.h> #include <paths.h>
#include <regex.h> #include <regex.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <unistd.h> #include <unistd.h>
#include "pathnames.h" #include "pathnames.h"
#define BASE_DECIMAL 10
jbeichUnsubmitted
Not Done
Inline Actions

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);

http://bxr.su/search?refs=strtou*&project=FreeBSD

jbeich: Why? Other uses of `strtou*()` in the file or across the base don't use macros for integer…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

This was recommended by davide@
I can change all of the references, or remove the macro.

allanjude: This was recommended by davide@ I can change all of the references, or remove the macro.
jbeichUnsubmitted
Not Done
Inline Actions

@davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback here, in the bug or on freebsd-current maillist.

jbeich: @davide has phabricator account. Let's ask him for the the rationale. I don't see his feedback…
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

Yeah, it was on IRC:
<davide> AllanJude: you don't do upper bound validation on the number of processes
<davide> AllanJude: '10' is hardcoded and makes hard to understand what that's actually about
<AllanJude> the 10 is the 'base'
<AllanJude> it is a standard argument to strtol
<davide> AllanJude: I can read the manpage
<davide> AllanJude: it should be a macro
<AllanJude> there is a macro for decimal?
<davide> #define BLAH 10 ?
<AllanJude> so, I should make one?
<davide> otherwise, lgtm

allanjude: Yeah, it was on IRC: <davide> AllanJude: you don't do upper bound validation on the number of…
mjgUnsubmitted
Not Done
Inline Actions

I also don't see what's the point of of this deifne.

mjg: I also don't see what's the point of of this deifne.
static void parse_input(int, char *[]); static void parse_input(int, char *[]);
static void prerun(int, char *[]); static void prerun(int, char *[]);
static int prompt(void); static int prompt(void);
static void run(char **); static void run(char **);
static void usage(void); static void usage(void);
void strnsubst(char **, const char *, const char *, size_t); void strnsubst(char **, const char *, const char *, size_t);
static pid_t xwait(int block, int *status); static pid_t xwait(int block, int *status);
static void xexit(const char *, const int); static void xexit(const char *, const int);
Show All 21 Lines
extern char **environ; extern char **environ;
int int
main(int argc, char *argv[]) main(int argc, char *argv[])
{ {
long arg_max; long arg_max;
int ch, Jflag, nargs, nflag, nline; int ch, Jflag, nargs, nflag, nline;
size_t linelen; size_t linelen;
size_t proclen;
mjgUnsubmitted
Not Done
Inline Actions

I would rename this to rl or rlimit, as a nit.

mjg: I would rename this to rl or rlimit, as a nit.
int proclim;
char *endptr; char *endptr;
inpline = replstr = NULL; inpline = replstr = NULL;
ep = environ; ep = environ;
eofstr = ""; eofstr = "";
Jflag = nflag = 0; Jflag = nflag = 0;
mjgUnsubmitted
Not Done
Inline Actions

Why is this set here?

mjg: Why is this set here?
(void)setlocale(LC_ALL, ""); (void)setlocale(LC_ALL, "");
/* /*
* POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that * POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that
* caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given * caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given
* that the smallest argument is 2 bytes in length, this means that * that the smallest argument is 2 bytes in length, this means that
* the number of arguments is limited to: * the number of arguments is limited to:
* *
Show All 36 Lines case 'n':
nflag = 1; nflag = 1;
if ((nargs = atoi(optarg)) <= 0) if ((nargs = atoi(optarg)) <= 0)
errx(1, "illegal argument count"); errx(1, "illegal argument count");
break; break;
case 'o': case 'o':
oflag = 1; oflag = 1;
break; break;
case 'P': case 'P':
if ((maxprocs = atoi(optarg)) <= 0) proclen = sizeof(proclim);
errx(1, "max. processes must be >0"); sysctlbyname("kern.maxproc", &proclim, &proclen, NULL, 0);
jbeichUnsubmitted
Done
Inline Actions

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
jbeich: Why allow value larger than `RLIMIT_NPROC` or `_SC_CHILD_MAX`? Soft limit would cap number of…
maxprocs = strtol(optarg, &endptr, BASE_DECIMAL);
if (*endptr != '\0')
errx(1, "max processes must be a number");
allanjudeAuthorUnsubmitted
Not Done
Inline Actions

We should probably use the same sysctl instead of nargs for the upper bounds as well

allanjude: We should probably use the same sysctl instead of nargs for the upper bounds as well
if (maxprocs >= proclim)
errx(1, "max processes must be less than kern.maxproc: %d", proclim);
bjkUnsubmitted
Done
Inline Actions

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.

bjk: I do not see how nargs is the correct value to use here; it has a different meaning to the…
lifanov_mail.lifanov.comUnsubmitted
Done
Inline Actions

Good point. I didn't trip this up by pure coincidence.
Let me think of a different value to use.

lifanov_mail.lifanov.com: Good point. I didn't trip this up by pure coincidence. Let me think of a different value to use.
lifanov_mail.lifanov.comUnsubmitted
Not Done
Inline Actions

RLIMIT_NPROC won't mean anything to the end user.
How about changing the line to just:
errx(1, "max processes must be less than %d", proclim);?

lifanov_mail.lifanov.com: RLIMIT_NPROC won't mean anything to the end user. How about changing the line to just: errx(1…
if (maxprocs < 1)
maxprocs = proclim;
break; break;
mjgUnsubmitted
Not Done
Inline Actions

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.

mjg: The code form user perspective should behave just like gnu xargs, unless there is a good reason…
case 'p': case 'p':
pflag = 1; pflag = 1;
break; break;
case 'R': case 'R':
Rflag = strtol(optarg, &endptr, 10); Rflag = strtol(optarg, &endptr, 10);
if (*endptr != '\0') if (*endptr != '\0')
errx(1, "replacements must be a number"); errx(1, "replacements must be a number");
break; break;
▲ Show 20 Lines • Show All 600 Lines • Show Last 20 Lines
View Options

bin/ls/util.c

Loading...