From owner-cvs-all@FreeBSD.ORG Sun Jan 9 22:14:24 2005 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 672F516A4CE; Sun, 9 Jan 2005 22:14:24 +0000 (GMT) Received: from rosebud.otenet.gr (rosebud.otenet.gr [195.170.0.26]) by mx1.FreeBSD.org (Postfix) with ESMTP id 0A3FE43D2F; Sun, 9 Jan 2005 22:14:20 +0000 (GMT) (envelope-from keramida@ceid.upatras.gr) Received: from gothmog.gr (patr530-a131.otenet.gr [212.205.215.131]) j09MEFhH003144; Mon, 10 Jan 2005 00:14:16 +0200 Received: from gothmog.gr (gothmog [127.0.0.1]) by gothmog.gr (8.13.1/8.13.1) with ESMTP id j09MEDhJ014545; Mon, 10 Jan 2005 00:14:14 +0200 (EET) (envelope-from keramida@ceid.upatras.gr) Received: (from giorgos@localhost) by gothmog.gr (8.13.1/8.13.1/Submit) id j09MEBJK014524; Mon, 10 Jan 2005 00:14:11 +0200 (EET) (envelope-from keramida@ceid.upatras.gr) Date: Mon, 10 Jan 2005 00:14:11 +0200 From: Giorgos Keramidas To: Bruce Evans Message-ID: <20050109221411.GC832@gothmog.gr> References: <200501031903.j03J3eBd044669@repoman.freebsd.org> <41D9A839.6000800@freebsd.org> <20050105150917.GA1592@orion.daedalusnetworks.priv> <20050110003627.T28619@delplex.bde.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20050110003627.T28619@delplex.bde.org> cc: Robert Watson cc: src-committers@freebsd.org cc: Scott Long cc: cvs-all@freebsd.org cc: cvs-src@freebsd.org Subject: Re: cvs commit: src/sbin/badsect badsect.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 09 Jan 2005 22:14:24 -0000 On 2005-01-10 03:36, Bruce Evans 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 > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -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