[PATCH] fdisk.c: major whitespace/style cleanup

Rob Landley rob at landley.net
Fri Feb 24 19:47:37 UTC 2006


On Friday 24 February 2006 12:12 pm, Vladimir N. Oleynik wrote:
> Rob,
>
> >>I'd like to work on making fdisk.c smaller and saner.
> >>But it needs a style cleanup first.
>
> Why first?

He's offering to clean it up, something I've previously stated on the list I 
think would be a very good thing.  (It's a mess.)

> If your path contains not only whitespace correction, require in
> addition to inform:
>
> -  sync ();
> -  sleep (4);
> +       sync();
> +       sleep(4); /* What? */
>
> Ask mainstream maintainer.

sync() is supposed to block until the data written before it is committed to 
disk.  If it doesn't, this is a kernel problem.  Sleeping after sync can only 
work around a kernel bug, and if it's a kernel before 2.4 we don't really 
support that as a deployment environment for new versions of busybox.

> For me best if 
> sync(); sleep(1); sync();
> but +1 sysclall.

It's best not to have the sleep at _all_ if it hasn't been needed since Linux 
1.2.  The code gives us no way of knowing, but on a 2.4 or 2.6 kernel sync() 
should work reliably.  If it doesn't, that would be a problem in the kernel 
and would need to be _fixed_, not worked around.

> >>One obvious bug spotted. Infinite loop:
> >>
> >>edit_int(int def, char *mesg)
> >>{
> >>    do {
> >>        fputs(mesg, stdout);
> >>        printf(" (%d): ", def);
> >>        if (!read_line())
> >>            return def;
> >>    }
> >>    while (!isdigit(*line_ptr));    /* FIXME: ?!! */
> >>    return atoi(line_ptr);
> >>}
>
> Why Infinite loop? read_line rewrote line_ptr

It's not advancing.  If *line_pt is not a digit, the loop will loop 
indefinitely, re-checking the same location over and over.

> > Applied.
>
> Can remove in general to me fdisk from the busybox?
> Also do that want. I of my work did not give the under this GPL licenses.
> I asked to not touch my utilities.

He's offering to clean up fdisk.  The current fdisk is a disaster.

You may remember an entire thread on this back in November?

http://www.busybox.net/lists/busybox/2005-November/017083.html

I see from that that Rob Sullivan also offered to help clean this thing up...

> Ohh. Rob. After reception of a maintainer you became completely deranged.
> :((

Nah, I was completely deranged before.

We have a philosophical difference.  I believe that porting "mainstream" apps 
to busybox verbatim is not a good idea.  If the "mainstream" app is so good, 
why not just use that instead?

The busybox version should be a small and simple implementation.  In the case 
of fdisk, the common case is that it writes a boot sector, which is a subset 
of a 512 byte data structure.  Taking 1000 lines of code to do this is 
questionable.  Taking 5000 lines of code to do this is very bad.

Saying "mainstream does it that way" is not an indicator of technical merit 
for a busybox applet.

> --w
> vodz

Rob
-- 
Never bet against the cheap plastic solution.



More information about the busybox mailing list