[BusyBox] pre-commit feedback, please

[Rob Landley]

You wait until erik is on vacation before posting this. :)

On Thursday 28 July 2005 16:44, Paul Fox wrote:
> 0000003 03-16-05 [PATCH] Do not export all make variables
>     [ submitter pkj was going to commit this.  see also bug #94 ]

I'm not good with makefiles.  No comment.

> 0000004 03-16-05 patch for httpd to support PHP CGI mode
>     [ questionable PHP-specific CGI variable.  see
> http://bugs.php.net/bug.php?id=28227 ]

I don't use PGP.  Again, no sane opinion I can really offer here...

> 0000008 03-16-05 modprobe applet is dependent on having a shell
>     [ patch and new code are big enough that i think this should
>        be configurable ]

Nah, not configurable.  If we're going to do this, rip out the shell dependent 
method entirely.

The patch seems generally sane.  How much size does it add?

Most of the new size seems to be the fork/exec handling via switch statement, 
and it seems there should be some kind of bb_fork_exec_wait() helper function 
since this isn't actually an uncommon thing to want to do.  (Have it return 
-1 if it couldn't fork, -2 if it couldn't exec, or the return code from 
wait() otherwise.)  You'd think there would already be one, but currently 
libbb just has run_parts.c and vfork_daemon_rexec.c, both of which are 
confusing enough that I'm not going down that particular rathole right now.  
There's a potential cleanup here, and it would be good to do it now rather 
than later.

> 0000024 03-16-05 patch: allow init to set controlling tty
>     [ my bug/patch -- the need for this has come up a few times
>        on the list in the last year, but no one has ever said yea or
>        nay on this feature.  ]

I have an whole init rewrite pending (after mount; I did the rewrite during 
the 1.0-pre feature freeze and lost it in the ensuing year.  Need to recreate 
it.)

That said, don't let the rewrite stand in the way of checking in something 
that sounds like it would be an improvement over what's there.  My standards 
for patching the current init are _really_ low, because I intend to throw 
most of it out when I get around to it.  (There's always the possibility that 
by the time I get around to redoing the rewrite, the current init will be 
shiny and happy and perfect.  See "pigs, flying" for more details.)

> 0000025 03-24-05 vi-editing mode for ash
>     [ my bug/patch -- i may be the only person who wants this,
>  but if there's anyone else like me, it'll make them _really_
>  happy. ;-) ]

As long as it's got a CONFIG_ option, sure.  (Didn't review the actual patch 
on this one.)

> 0000028 03-16-05 patch: new setsid applet
>     [ my bug/patch -- i hope this is non-controversial ]

The usage doesn't really say how to use it.

This is another case for a fork/exec helper, although in this case the parent 
returns immediately (and exits).  (Sounds like it needs a wait/nowait 
flag...)  Among other things: if you typo the command name setsid is to run, 
the error message will be "execvp".  A helper could afford to budget a few 
more bytes on a better "could not exec %s" error message...

Other than that, the patch looks nice and straightforward.  No objection here 
(for 1.1).

> 0000037 03-16-05 patch: allow suppression of default client-id
>     [ my bug/patch -- makes udhcpc more compatibile with both
>        u-boot and the linux kernel ]

Actually I'd call this a bugfix and would consider it for 1.0.1.  (It might be 
possible to be a bit more clever and squeeze out an extra byte or two, but 
I'm not going to quibble just now.  The dhcp subdirectory isn't an area I've 
meddled with...)

> 0000046 03-16-05 Config Applet
>     [ new applet -- displays busybox config options.  i'm not sure i
>  understand why this is needed.  ]

Me neither.

> 0000071 03-16-05 patch: implement "--color" option for ls coloring control
>     [ my bug/patch -- this has been hashed over on the list.  can i commit?
> ]

As long as it only impacts ls when coloring is enabled anyway, go for it.  (I 
object in principle to additional config options for small tweaks, but I'm 
going to look the other way in this case.  Sane default, I can ignore it, 
ok.)

> 0000072 03-16-05 Add applet to redirect console output via ioctl(...,
> TIOCCONS) [ looks okay to me ]

A) The patch looks mostly reasonable.  I have a few small gripes:

1) set the tabstop to something other than 4 and look at the else in 
getcons_main.  It's spaces, not tab.

2) open/perror/return is not bb_open why?

3) It seems like there should be a way to shrink command line option parsing 
so there aren't two calls to bb_show_usage() and two tests for 
OPT_GETCONS_RESET...

But those aren't show-stoppers.

B) We need a menuconfig sub-menu for "things unique to busybox".  
(pipe_progress comes to mind, although I preferred the name "count"...)

So my only serious objection to this is it should be in such a menuconfig 
division and whoever checks this in should find the other stuff to move into 
that menu. :)

> 0000073 03-16-05 Add option to inetd applet to run in foreground
>     [ looks okay to me -- option already present on uclinux ]

I agree this has nothing to do with uclinux.  Go for it.

> 0000094 03-16-05 Busybox 'make install' does not respect PREFIX when using
> O= [ see also bug #3 -- if everything's exported, why is this needed? ]

Makefile tweak, not my thing.  No opinion.

> 0000115 03-25-05 ifenslave
>     [ new applet ]

Sure, why not?

> 0000132 03-16-05 Implement fork using longjmp
>     [ somewhat controversial, i recall... ]

It is.  This is a libc thing, not a busybox thing.  I object to having it in 
busybox on that grounds.

That said: the whole mess of figuring out when we need to vfork and when we 
need a real fork is a bit of a nightmare right now anyway.  If we wanted to 
clean up ALL that (I could see "use vfork instead of real fork" under 
"general configuration" next to "free memory before exiting"), and add this 
as a third option to a three way (fork/vfork/longjmp) selector that selects a 
bb_spork() helper function in its own darn file, then it becomes a lot more 
palatable.

This could tangentially be related to the "compiler.h" cleanup I mentioned 
earlier, and the fork/exec/wait combo helper function.  I.E. this is 1.1 
material, but it's not a no-brainer to check in in its present form.

I'd be willing to help with this cleanup _after_ I get the mount rewrite in 
and 1.0.1-final out the door.

Rob