Date: Tue, 25 Jan 2011 18:10:51 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Maxim Sobolev <sobomax@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r217808 - head/sbin/fdisk Message-ID: <20110125172518.G1400@besplex.bde.org> In-Reply-To: <201101250435.p0P4Z7iW017380@svn.freebsd.org> References: <201101250435.p0P4Z7iW017380@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110125172518.G1400>