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

[Glenn McGrath]

On Mon, 06 Oct 2003 14:40:47 +0400
"Vladimir N. Oleynik" <dzo at simtreas.ru> wrote:

> Glenn,
> 
> > Attached is a patch that adds the -n option to xargs, and it should
> > be easy to extend it further.
> 
> Heh. You make my work ;-)
> I attached new version with support "-n max_arg" but for pre-previous
> CVS verions. I like not your changes:
> 
> 1.
> 
>   + * BUGS: -p doesnt accept user input
> + *
> 
> Really?

The spec says input is supposed to come from /dev/tty, often filenames
for xargs comes from stdin, so user input for confirmation cannot.

e.g.
$ echo "./README" | xargs -p ls -l
ls -l ./README ?...y
-rw-r--r--    1 bug1     users        4724 Aug  9 09:41 ./README

$ echo "./README" | ./busybox xargs -p ls -l
ls -l ./README  ?...bug1 at home busybox $

But this is a bad example because i demonstrate my patch to xargs breaks
something else.

$ echo "README" | ./busybox xargs ls -al
ls: README : No such file or directory

$ ls README
README

> ./busybox xargs -p
> 1
> /bin/echo 1 ?...y
> 1
> 
> Work fine. And you revrote absoluteli same algorithm. but with code
> bigger.

But more features !

I committed a newer patch to cvs already, i will have to try and merge your changes

> 2. 	
>   	/* allocating pointers for execvp: a*arg, arg from stdin, NULL
>   	*/
> -	args = xcalloc(a + 3, sizeof(char *));
> +	args = xcalloc(a + 2, sizeof(char *));
> 
> No! See comment.

Maybe i dont understand your comment ?

a*arg,  a * sizeof(char *)
arg from stdin, 1 * sizeof(char *)
NULL pointer, 1 * sizeof(char *)

equals (a + 1 + 1) * sizeof(char *)
	xcalloc(a + 2, sizeof(char *))


> +if (wpid == -1 && errno == ECHILD) {
> +  	                         /* we missed its termination */
> +  	                         break;
> +  	                 }
> 
> 
> Why?! We generate ONE child and wait die her. This code not required.

Im not sure where this code comes from.



Glenn