Changeset View
Standalone View
usr.bin/xargs/xargs.c
Show All 40 Lines | |||||
#ifndef lint | #ifndef lint | ||||
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/types.h> | ||||
#include <sys/wait.h> | #include <sys/wait.h> | ||||
#include <sys/time.h> | |||||
jbeich: style(9) doesn't allow both `<sys/types.h>` and `<sys/param.h>` included at the same time. | |||||
Not Done Inline Actionssys/types.h must be included prior to sys/wait.h. mjg: sys/types.h must be included prior to sys/wait.h. | |||||
#include <sys/resource.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" | ||||
static void parse_input(int, char *[]); | static void parse_input(int, char *[]); | ||||
Not Done Inline ActionsWhy? 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); jbeich: Why? Other uses of `strtou*()` in the file or across the base don't use macros for integer… | |||||
Not Done Inline ActionsThis was recommended by davide@ allanjude: This was recommended by davide@
I can change all of the references, or remove the macro. | |||||
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… | |||||
Not Done Inline ActionsYeah, it was on IRC: allanjude: Yeah, it was on IRC:
<davide> AllanJude: you don't do upper bound validation on the number of… | |||||
Not Done Inline ActionsI 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 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); | ||||
static void waitchildren(const char *, int); | static void waitchildren(const char *, int); | ||||
Show All 20 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; | ||||
struct rlimit rl; | |||||
Not Done Inline ActionsI would rename this to rl or rlimit, as a nit. mjg: I would rename this to rl or rlimit, as a nit. | |||||
char *endptr; | char *endptr; | ||||
const char *errstr; | const char *errstr; | ||||
inpline = replstr = NULL; | inpline = replstr = NULL; | ||||
ep = environ; | ep = environ; | ||||
eofstr = ""; | eofstr = ""; | ||||
Jflag = nflag = 0; | Jflag = nflag = 0; | ||||
Not Done Inline ActionsWhy 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 40 Lines | case 'n': | ||||
if (errstr) | if (errstr) | ||||
errx(1, "-n %s: %s", optarg, errstr); | errx(1, "-n %s: %s", optarg, errstr); | ||||
break; | break; | ||||
case 'o': | case 'o': | ||||
oflag = 1; | oflag = 1; | ||||
break; | break; | ||||
case 'P': | case 'P': | ||||
maxprocs = strtonum(optarg, 1, INT_MAX, &errstr); | maxprocs = strtonum(optarg, 1, INT_MAX, &errstr); | ||||
if (errstr) | if (errstr) | ||||
Done Inline ActionsWhy 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… | |||||
errx(1, "-P %s: %s", optarg, errstr); | errx(1, "-P %s: %s", optarg, errstr); | ||||
if (getrlimit(RLIMIT_NPROC, &rl) != 0) | |||||
errx(1, "getrlimit failed"); | |||||
Not Done Inline ActionsWe 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 (*endptr != '\0') | |||||
errx(1, "invalid number for -P option"); | |||||
Done Inline ActionsI 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… | |||||
Done Inline ActionsGood point. I didn't trip this up by pure coincidence. lifanov_mail.lifanov.com: Good point. I didn't trip this up by pure coincidence.
Let me think of a different value to use. | |||||
Not Done Inline ActionsRLIMIT_NPROC won't mean anything to the end user. 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 < 0) | |||||
errx(1, "value for -P option should be >= 0"); | |||||
if (maxprocs == 0 || maxprocs > rl.rlim_cur) | |||||
maxprocs = rl.rlim_cur; | |||||
break; | break; | ||||
Not Done Inline ActionsThe 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 602 Lines • Show Last 20 Lines |
style(9) doesn't allow both <sys/types.h> and <sys/param.h> included at the same time.