From owner-svn-src-all@FreeBSD.ORG Tue Jan 25 07:10:55 2011 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id DD83E106564A; Tue, 25 Jan 2011 07:10:55 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail02.syd.optusnet.com.au (mail02.syd.optusnet.com.au [211.29.132.183]) by mx1.freebsd.org (Postfix) with ESMTP id 62F1F8FC0C; Tue, 25 Jan 2011 07:10:54 +0000 (UTC) Received: from c122-106-165-206.carlnfd1.nsw.optusnet.com.au (c122-106-165-206.carlnfd1.nsw.optusnet.com.au [122.106.165.206]) by mail02.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id p0P7Apt3020572 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 25 Jan 2011 18:10:52 +1100 Date: Tue, 25 Jan 2011 18:10:51 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Maxim Sobolev In-Reply-To: <201101250435.p0P4Z7iW017380@svn.freebsd.org> Message-ID: <20110125172518.G1400@besplex.bde.org> References: <201101250435.p0P4Z7iW017380@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r217808 - head/sbin/fdisk X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Jan 2011 07:10:56 -0000 On Tue, 25 Jan 2011, Maxim Sobolev wrote: > Log: > Supply maximum value as an argument to the decimal() function > instead of supplying number of bits. > > Submitted by: bde Thaks. I would have spelled this quite differently, but this is functional and fairly easy to clean. > Modified: head/sbin/fdisk/fdisk.c > ============================================================================== > --- head/sbin/fdisk/fdisk.c Tue Jan 25 01:26:25 2011 (r217807) > +++ head/sbin/fdisk/fdisk.c Tue Jan 25 04:35:07 2011 (r217808) > @@ -49,7 +49,10 @@ __FBSDID("$FreeBSD$"); > > int iotest; > > -#define NOSECTORS ((u_int32_t)-1) > +#define NO_DISK_SECTORS ((u_int32_t)-1) > +#define NO_TRACK_CYLINDERS 1023 > +#define NO_TRACK_HEADS 255 > +#define NO_TRACK_SECTORS 63 "NO" looks like negation but means "number of", and these aren't even numbers of things -- they the maximum values of things, and thus one less than the numbers of things, except for sectors since those are numbered starting at 1. I would have expanded these values in all ~2 places each that they are used. "NOSECTORS" seems to have been abused as a magic in-band error value. Its use under a new name as a maximum non-error value conflicts at least logically with this. > @@ -572,9 +575,9 @@ change_part(int i) > } > > do { > - Decimal("sysid (165=FreeBSD)", partp->dp_typ, tmp, sizeof(partp->dp_typ) * 8); > - Decimal("start", partp->dp_start, tmp, sizeof(partp->dp_start) * 8); > - Decimal("size", partp->dp_size, tmp, sizeof(partp->dp_size) * 8); > + Decimal("sysid (165=FreeBSD)", partp->dp_typ, tmp, 255); > + Decimal("start", partp->dp_start, tmp, NO_DISK_SECTORS); > + Decimal("size", partp->dp_size, tmp, NO_DISK_SECTORS); The start is an offset, and thus its maximum is 1 less than the maximum number of disk sectors, so the off-by-1 error in NO_DISK_SECTORS gives the correct result for it. The size is a count, and thus its maximum is the number of disk sectors, which should be settable to 1 larger than the largest offset. However 1 more is not representable in dp_size, so the maximum must be limited to 1 less than it should be; thus NO_DISK_SECTORS matches its name and doesn't have an off-by-1 error for this use. > @@ -586,9 +589,9 @@ change_part(int i) > tcyl = DPCYL(partp->dp_scyl,partp->dp_ssect); > thd = partp->dp_shd; > tsec = DPSECT(partp->dp_ssect); > - Decimal("beginning cylinder", tcyl, tmp, 10); > - Decimal("beginning head", thd, tmp, sizeof(partp->dp_shd) * 8); > - Decimal("beginning sector", tsec, tmp, 6); > + Decimal("beginning cylinder", tcyl, tmp, NO_TRACK_CYLINDERS); > + Decimal("beginning head", thd, tmp, NO_TRACK_HEADS); > + Decimal("beginning sector", tsec, tmp, NO_TRACK_SECTORS); Spelling NO_TRACK_CYLINDERS as 1023 and NO_TRACK_SECTORS as 63 would fix the style bugs (lines too long) and hopefully reduce confusion between the maximum offset and the total count. The distinction between these is subtlest for sectors since their values are the same. > @@ -915,16 +918,12 @@ ok(const char *str) > } > > static int > -decimal(const char *str, int *num, int deflt, int nbits) > +decimal(const char *str, int *num, int deflt, uint32_t maxval) > { > - long long acc = 0, limit; > + long long acc = 0; I would never use the long long abomination. Now maxval has a reasonably correct type, the same type for the accumulator would work once the limit checking is changed slightly. > @@ -941,7 +940,7 @@ decimal(const char *str, int *num, int d > return 0; > while ((c = *cp++)) { > if (c <= '9' && c >= '0') { > - if (acc < limit) > + if (maxval > 0 && acc <= maxval) > acc = acc * 10 + c - '0'; `acc' can grow to about 10 times larger than the limit, and overflow would occur and break the limit checking if `acc' were uint32_t. Using the long long abomination avoids this overflow. It is better to break when the new `acc' would exceed the limit. See strtol.c for this. But this should just use strtoul() and not be restricted to decimal input. fdisk also has a home-made non-decimal input routine for sectors (like dd(1) parameter parsing and parse^Wexpand_number(3)). This uses strtoul() internally). > @@ -949,10 +948,11 @@ decimal(const char *str, int *num, int d > if (c == ' ' || c == '\t') > while ((c = *cp) && (c == ' ' || c == '\t')) cp++; > if (!c) { > - if (acc >= limit) { > - acc = limit - 1; > - printf("%s is too big, it will be truncated to %lld\n", > - lbuf, acc); > + if (maxval > 0 && acc > maxval) { > + acc = maxval; > + printf("%s exceeds maximum value allowed for " > + "this field. The value has been reduced " > + "to %lld\n", lbuf, acc); I would never use C90 string concatenation to obfuscate a message like this. The output now has formatting errors instead of grammar errors. The obfuscation keeps source line lengths below the limit but is too easy to use to produce output line lengths exceeding the limit, as this now does. Here the output is fairly obiously too long since the statement is so ugly. It also uses an abnormal sentence break of 1 space. Another message in the same function uses the normal sentence break of 2 spaces. I would just print "%s exceeds the maximum of %u. Try again.\n" and restart, like the other error handling in this function does. Bruce