[BusyBox] [PATCH] eject umount patch 2

[Rob Landley]

On Friday 13 May 2005 08:50 am, Tito wrote:

> Hi, here goes patch n°2,

Generally looks good.  I tweaked it a bit (removed commented out CLOSE_TRAY 
define and one commented out occurrence of it, and I don't know why return 
success and exit success were different things in the #ifdef...)

> now we have some kind of 
> input validation so the system call should not be a problem.

Ooh, hadn't thought of find_mount_point.  Good idea.

Although the unmount would still fail if we had whitespace or quote characters 
in any of the path elements, which is technically possible.  
(find_mount_point would be happy, but the call to system would barf of the 
arguments.)

The trivial fix is to put quotes around the argument, which gets rid of the 
whitespace problem (which people may actually hit).  There would still be a 
problem if the path name contained quotes or backslash characters of its own, 
but I'm not sure it's worth the size hit to scan through the string to escape 
them...

Imagine a gnome utility that mounts a CD in a directory with its imdb name to 
look good on the desktop, and then calling eject on that...

> > Writing to argv generally makes me nervous.
>
> Seems to me that  the getopt funcs do this without problems, so why should
> we not do?

It still makes me nervous.  Modifies ps output and all that, and here we're 
potentially writing past end of the list.  (I think there's a null terminator 
or some such there...)

> > As long as we're doing the
> > assignment anyway, we could probably get away with something like:
> > 	char *device=argv[optind] ? : DEFAULT_CDROM;
>
> I dont like this.... looks ugly...... but its true it saves some space ;-)
> Done.

? : is ugly at the best of times, but it's small and can be a good way to 
localize decision-making logic...

> > I also think we should allocate the command memory on the stack (with
> > alloca), so we don't need a cleanup #ifdef.  (You know, there are macros
> > for memory allocations.  Nobody ever seems to _use_ them, but...)
>
> Now I'm using bb_xasprintf, so no macros.

Cool.

> For the global config option, i think it solves some problems but creates
> others so it is not worth the work.
> 1) there are applets that didn't have long options at all

Not a problem.

> 2) there are/were applets with some options that are only long-options

Yeah, problem.

> 3) its better if you can select it on the single applet so you can enable
>      them only where needed ( by a script or similar) saving some space.

The configuration granularity required for this is too much.  Beyond a certain 
point, we might as well just let them tweak the source code to cut certain 
kinds of stuff out.  We already have 8 gazillion options in menuconfig (they 
have to choose which applets they want), which can be a bit overwhelming.  
Having too many sub-options for each applet does not help matters; having so 
much configurability that no mortal can actually make proper use of it except 
the authors of the code is not a good thing.

> In this case my personal opinion is to remove --trayclose completely
> if nobody objects.

I think I agree.  The -t option is rarely used enough that having a long 
option for it is at least something we can hold off on until somebody 
complains about its absence. :)

Updated patch forthcoming...

> Ciao,
> Tito

Rob