[BusyBox] Re: [patch] Why busybox xargs is broken.
Erik Andersen
andersen at codepoet.org
Fri Oct 3 09:24:03 UTC 2003
On Thu Oct 02, 2003 at 06:33:09AM -0500, Rob Landley wrote:
> --- busybox-old/applets/busybox.c 2003-03-19 03:11:13.000000000 -0600
> +++ busybox/applets/busybox.c 2003-10-02 06:05:23.543865400 -0500
> @@ -98,7 +98,7 @@
>
> int busybox_main(int argc, char **argv)
> {
> - int col = 0, len, i;
> + int col = 0;
>
> #ifdef CONFIG_FEATURE_INSTALLER
> /*
> @@ -161,18 +161,7 @@
> /* Flag that we've been here already */
> been_there_done_that = 1;
>
> - /* Move the command line down a notch */
> - len = argv[argc] + strlen(argv[argc]) - argv[1];
> - memmove(argv[0], argv[1], len);
> - memset(argv[0] + len, 0, argv[1] - argv[0]);
> -
> - /* Fix up the argv pointers */
> - len = argv[1] - argv[0];
> - memmove(argv, argv + 1, sizeof(char *) * (argc + 1));
> - for (i = 0; i < argc; i++)
> - argv[i] -= len;
> -
> - return (main(argc, argv));
> + return (main(argc-1, argv+1));
> }
This stuff moving and memsetting stuff is here to modify the
output stuff displayed by 'ps' when busybox is invoked as
'busybox applet' making ps show 'applet'.
Admitedly, not the most critical feature. I suppose I should
have added a comment as to why it was doing all that copying.
> /*
> diff -ru busybox-old/findutils/Config.in busybox/findutils/Config.in
> --- busybox-old/findutils/Config.in 2003-07-14 14:36:56.000000000 -0500
> +++ busybox/findutils/Config.in 2003-10-02 05:58:16.165836736 -0500
> @@ -97,5 +97,12 @@
> xargs is used to execute a specified command on
> every item from standard input.
>
> +config CONFIG_FEATURE_XARGS_OPTIONS
> + bool " Enable command line options for xargs"
> + default n
> + depends on CONFIG_XARGS
> + help
> + Provides the -p, -t, -r, and -n options to xargs.
> +
> endmenu
This probably should not be a config option. If people want
xargs they probably want it to behave itself as expected.
> diff -ru busybox-old/findutils/xargs.c busybox/findutils/xargs.c
> --- busybox-old/findutils/xargs.c 2003-06-20 04:01:54.000000000 -0500
> +++ busybox/findutils/xargs.c 2003-10-02 06:11:29.689202848 -0500
> @@ -1,3 +1,4 @@
> +/* vi: set sw=4 ts=4: */
> /*
> * Mini xargs implementation for busybox
> * Only "-prt" options are supported in this version of xargs.
> @@ -24,6 +25,7 @@
>
> #include <stdio.h>
> #include <stdlib.h>
> +#include <string.h>
> #include <unistd.h>
> #include <getopt.h>
> #include <errno.h>
> @@ -36,80 +38,162 @@
> This function have special algorithm.
> Don`t use fork and include to main!
> */
> -static void xargs_exec(char * const * args)
> +static int xargs_exec(char * const * args)
> {
> int p;
> - int common[4]; /* shared vfork stack */
>
> - common[0] = 0;
> if ((p = vfork()) >= 0) {
> if (p == 0) {
> /* vfork -- child */
> execvp(args[0], args);
> - common[0] = errno; /* set error to shared stack */
> - _exit(1);
> + _exit(127);
> } else {
> + int retval;
> /* vfork -- parent */
> - wait(NULL);
> - if(common[0]) {
> - errno = common[0];
> - bb_perror_msg_and_die("%s", args[0]);
> - }
> + waitpid(p,&retval,0);
I recommend using just wait(&retval)
> + return retval>>8;
Ugh.
Please use WEXITSTATUS(retval). Also, this expression is only
guaranteed to be valid if WIFEXITED(retval) is true. If the
child catches a signal, or if some other child of the child
process exits, this will return garbage. Something like
if (WIFEXITED(retval)) {
return WEXITSTATUS(retval)
} else {
return 1;
}
> }
> } else {
> bb_perror_msg_and_die("vfork");
> }
> }
>
> +#define FLAG_XARGS_PROMPT 1
> +#define FLAG_XARGS_VERBOSE 2
> +#define FLAG_XARGS_NOBLANK 4
> +
> +struct cmdlist
> +{
> + char *cmd;
> + struct cmdlist *next;
> +};
> +
> +/* xargs -p (prompt) -t (verbose) -r (don't run if no command)
> + * -n (lines per command) */
> +
> int xargs_main(int argc, char **argv)
> {
> - char *file_to_act_on;
> - char **args;
> - int i, a;
> - char flg_vi; /* verbose |& interactive */
> - char flg_no_empty;
> -
> - bb_opt_complementaly = "pt";
> - a = bb_getopt_ulflags(argc, argv, "tpr");
> - flg_vi = a & 3;
> - flg_no_empty = a & 4;
> -
> - a = argc - optind;
> - argv += optind;
> - if(a==0) {
> - /* default behavior is to echo all the filenames */
> + int i, retval=0, command_count;
> + struct cmdlist *command_list=NULL;
> + char *temp, **args, **next_args;
> +
> +#ifdef CONFIG_FEATURE_XARGS_OPTIONS
> + int max_command_count=0;
> + char flags=0;
> +
> + /* Parse command line arguments (can't use optarg, don't want reordering) */
> +
> + for(i=1;i<argc;i++) {
> + if(argv[i][0]!='-') break;
> + for(command_count=1;argv[i][command_count];command_count++) {
> + switch(argv[i][command_count]) {
> + case 't':
> + flags|=FLAG_XARGS_VERBOSE;
> + break;
> + case 'p':
> + flags|=FLAG_XARGS_PROMPT;
> + break;
> + case 'r':
> + flags|=FLAG_XARGS_NOBLANK;
> + break;
> + case 'n':
> + if(++i!=argc) {
> + max_command_count=(int)bb_xgetularg10(argv[i]);
> + break;
> + }
> + /* fall through */
> + default:
> + bb_show_usage();
> + }
> + }
> + }
> +#else
> + i=1;
> +#endif
Ugh.
There is no need to reinvent getopt here. bb_getopt_ulflags is a
much more compact wrapper on top of getopt. It is admitedly not
well documented, but it works nicely and produces very compact
arg parsing. It should be something like:
char *tmp;
a = bb_getopt_ulflags(argc, argv, "tprn:", &tmp);
if (a & (1 << 0))
/* -t opt */
flags|=FLAG_XARGS_VERBOSE;
if (a & (1 << 1))
/* -p opt */
flags|=FLAG_XARGS_PROMPT;
if (a & (1 << 2))
/* -r opt */
flags|=FLAG_XARGS_NOBLANK;
if (a & (1 << 3))
/* -n opt */
max_command_count=(int)bb_xgetularg10(tmp);
> + /* Trim argument list to the command we're going to exec */
> + argc-=i;
> + argv+=i;
> +
> + /* If no command, default behavior is to echo all the filenames */
> + if(!argc) {
> *argv = "/bin/echo";
> - a++;
> + argc++;
> }
> - /* allocating pointers for execvp: a*arg, arg from stdin, NULL */
> - args = xcalloc(a + 3, sizeof(char *));
>
> - /* Store the command to be executed (taken from the command line) */
> - for (i = 0; i < a; i++)
> - args[i] = *argv++;
> -
> - /* Now, read in one line at a time from stdin, and store this
> - * line to be used later as an argument to the command */
> - while ((file_to_act_on = bb_get_chomped_line_from_file(stdin)) != NULL) {
> - if(file_to_act_on[0] != 0 || flg_no_empty == 0) {
> - args[a] = file_to_act_on[0] ? file_to_act_on : NULL;
> - if(flg_vi) {
> - for(i=0; args[i]; i++) {
> - if(i)
> - fputc(' ', stderr);
> - fputs(args[i], stderr);
> - }
> - fputs(((flg_vi & 2) ? " ?..." : "\n"), stderr);
> - }
> - if((flg_vi & 2) == 0 || bb_ask_confirmation() != 0 ) {
> - xargs_exec(args);
> + /* Read lines from stdin into a linked list. */
> + command_count=0;
> + while ((temp = bb_get_chomped_line_from_file(stdin)) != NULL) {
> + struct cmdlist *next_cmd=xcalloc(1,sizeof(struct cmdlist));
> + next_cmd->next=command_list;
> + next_cmd->cmd=temp;
> + command_list=next_cmd;
> + command_count++;
> + }
This doesn't need to be calloc'd... You could easily use
libbb/llist_add_to.c here for the linked list handling which
will save you some bother and should save some bytes as well.
-Erik
--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--
More information about the busybox
mailing list