[BusyBox] Re: [patch] Why busybox xargs is broken.

Rob Landley rob at landley.net
Fri Oct 3 20:26:17 UTC 2003


On Friday 03 October 2003 04:24, Erik Andersen wrote:

> 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.

A comment would be good, yes.  But why do you want to change what ps shows?  
When busybox is invoked as "busybox applet", the command you're running IS 
"busybox applet".  Why would you want it to show something else?

Python doesn't do this:

[landley at localhost landley]$ cat temp2
system "ps l"
[landley at localhost landley]$ perl temp2
F   UID   PID  PPID PRI  NI   VSZ  RSS WCHAN  STAT TTY        TIME COMMAND
0   500  1361  1360  15   0  4380 1460 sys_wa S    pts/1      0:00 /bin/bash
0   500  1438  1361  16   0  5300 1288 sys_wa S    pts/1      0:00 perl temp2
0   500  1439  1438  20   0  3436 1488 0      R    pts/1      0:00 ps l
[landley at localhost landley]$

Perl doesn't do this:

[landley at localhost landley]$ cat temp
import os
os.system("ps l")
[landley at localhost landley]$ python temp
F   UID   PID  PPID PRI  NI   VSZ  RSS WCHAN  STAT TTY        TIME COMMAND
0   500  1361  1360  15   0  4380 1460 sys_wa S    pts/1      0:00 /bin/bash
0   500  1399  1361  17   0  2804 1708 sys_wa S    pts/1      0:00 python temp
0   500  1400  1399  18   0  3436 1488 0      R    pts/1      0:00 ps l
[landley at localhost landley]$

Both have a second method of invocation (where the script has the execute bit 
and starts with #!/usr/bin/python) that show the running command as the 
script name.  Similarly, busybox has the symlink behavior.  When you're 
explicitly running busybox, why shouldn't it say so?

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

Okay.  I had problems with it circa 2.4.4, but I suspect it was a kernel bug 
that's been fixed...

> > +			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;
>     }

Okay.

> >  		}
> >  	} 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.

I was doing that because "xargs ls -l" is not the same thing as "xargs -l ls".  
The only way I saw at the time to switch off (glibc) getopt's argument 
reordering behavior was to set an environment variable, but I now see that 
the option string can start with "+".

>  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.

I spent half an hour trying to figure out what it was trying to do.

These are the first four comment lines:
	You can set bb_opt_complementaly as string with one or more
	complementaly or incongruously options.
	If sequential founded option haved from this string
	then your incongruously pairs unsets and complementaly make add sets.

First off, what's the difference between a "complementaly" option, an 
"incongruously" option, and a regular option?  (I don't know what this IS, 
but it involves setting undocumented global variables rather than simply 
passing a parameter.  It's possible that this is a size optimization, but it 
should be documented if so.)

Secondly, I've read the second two lines of that over a dozen times now.  What 
does it mean?

>  It should be something like:
>
> 	char *tmp;
> 	a = bb_getopt_ulflags(argc, argv, "tprn:", &tmp);

Make that "+tprn:".  Option reording in xargs is bad, the command you're 
running can have options.

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

At that point I'd just make the #defines match up with the a arguments and set 
flags directly to the return value of bb_getopt_ulflags.  (The fact I'm using 
a bitfield like that is actually residue from my earlier attempt to make 
bb_getopt_ulflags work, before my brain melted trying to read it.  I left it 
because the bitfield is probably smaller anyway, although I haven't 
checked...)

> > +	/* 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.

Cool.  (I didn't know there was one.  Did I miss docs, or should I start 
writing some? :)

>  -Erik

And now you know why I didn't want CVS access yet. :)

I'll fix it up this evening and submit a fresh patch...

Rob



More information about the busybox mailing list