[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