cvs commit: src/sbin/badsect badsect.c

[Giorgos Keramidas]

On 2005-01-10 03:36, Bruce Evans <bde at zeta.org.au> wrote:
>On Wed, 5 Jan 2005, Giorgos Keramidas wrote:
>>>>  Revision  Changes    Path
>>>>  1.21      +4 -1      src/sbin/badsect/badsect.c
>>>
>>> I'd argue that atol() is buggy for a number of other reasons also,
>>> mainly that it means that you're limited to 2^31 sectors.  Maybe this
>>> should change to strtoull() ?
>>
>> Ideally, if that's not a problem, we should go for uintmax_t, since
>> `number' in this case is then stored in a daddr_t, which is 64-bits
>> wide.
>
> Large block numbers still wouldn't actually work.
[snip useful explanation]

Thanks!  That was truly enlightening.

> > How about something like this?  I could find no easy way to check for
> > overflow of daddr_t, so this is still not quite perfect but it's an
> > improvement that may help with Really-Huge(TM) disks.
> >
> > %%%
> > Index: badsect.c
> > ===================================================================
> > RCS file: /home/ncvs/src/sbin/badsect/badsect.c,v
> > retrieving revision 1.21
> > diff -u -u -r1.21 badsect.c
> > --- badsect.c	3 Jan 2005 19:03:40 -0000	1.21
> > +++ badsect.c	5 Jan 2005 15:03:39 -0000
> > @@ -62,6 +62,7 @@
> >  #include <errno.h>
> >  #include <dirent.h>
> >  #include <fcntl.h>
> > +#include <inttypes.h>
> >  #include <libufs.h>
> >  #include <paths.h>
> >  #include <stdio.h>
> > @@ -94,6 +95,7 @@
> >  	DIR *dirp;
> >  	char name[2 * MAXPATHLEN];
> >  	char *name_dir_end;
> > +	char *errp;
>
> Style bugs:
>
> (1) Unsorting of declarations.
> (2) Declarations of the same type not on the same line.

I have to admit I didn't really pay attention to style(9) while writing
this.  I also noticed a few bugs in the handling of strtol() errors, but
then forgot about the post.

> > @@ -124,8 +126,11 @@
> >  			err(7, "%s", name);
> >  	}
> >  	for (argc -= 2, argv += 2; argc > 0; argc--, argv++) {
> > -		number = strtol(*argv, NULL, 0);
> > -		if (errno == EINVAL || errno == ERANGE)
> > +		errp = NULL;
>
> The variable pointed to by strto*()'s second parameter is an output
> parameter; initializing it in the caller is bogus.

Are strto*() functions always supposed to set endp to _something_, or is
there a case for them to attempt saving a few cycles by leaving it
untouched, possibly set to garbage?  This is what I was trying to avoid.

> > +		errno = 0;
>
> This initialization is necessary but is missing in the current version
> (rev.1.21).

I know.  This can easily be fixed :-)

>> +		number = strtoumax(*argv, &errp, 0);
>> +		if (errno != 0 || errp != NULL || (errp != NULL &&
>
> `errp' is guaranteed to be non-NULL here, so this check causes badsect
> to always fail.  This bug may have been caused by the bad variable name
> `errp'.  It is a "next" pointer, not an "error" pointer.

I got bitten by this when I tested badsect locally.

> A correct and portable strto*() check looks something like:
>
> 		if (errno != 0 || errp == *argv || *errp != '\0)

Thanks, that makes sense :-)

- Giorgos