Date: Mon, 10 Jan 2005 00:14:11 +0200 From: Giorgos Keramidas <keramida@ceid.upatras.gr> To: Bruce Evans <bde@zeta.org.au> Cc: cvs-src@freebsd.org Subject: Re: cvs commit: src/sbin/badsect badsect.c Message-ID: <20050109221411.GC832@gothmog.gr> In-Reply-To: <20050110003627.T28619@delplex.bde.org> References: <200501031903.j03J3eBd044669@repoman.freebsd.org> <41D9A839.6000800@freebsd.org> <20050105150917.GA1592@orion.daedalusnetworks.priv> <20050110003627.T28619@delplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2005-01-10 03:36, Bruce Evans <bde@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050109221411.GC832>