Date: Sat, 16 Nov 2002 13:44:47 -0800 (PST) From: Julian Elischer <julian@elischer.org> To: Bruce Evans <bde@zeta.org.au> Cc: Tim Robbins <tjr@FreeBSD.ORG>, FreeBSD current users <current@FreeBSD.ORG> Subject: Re: Sign fixes for disklabel(8) Message-ID: <Pine.BSF.4.21.0211161329240.53326-100000@InterJet.elischer.org> In-Reply-To: <20021116194439.E17742-100000@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 16 Nov 2002, Bruce Evans wrote:
> On Sat, 16 Nov 2002, Tim Robbins wrote:
>
> > On Fri, Nov 15, 2002 at 03:59:25PM -0800, Julian Elischer wrote:
> >
> > > Here are the diffs to allow disklabel to correctly create partitions >
> > > 1TB (up to 2TB is useful with UFS2) pending a different partitionning
> > > scheme. It also allows you to correctly make smaller partitions beyond
> > > 1TB which is nice if you don't want to waste 800GB on an array :-)
>
> Labels should be able to handle disks much larger than 1TB (under -current
> where there is no significant daddr_t limit) by fudging the sector size.
> E.g., with a sector size of 8K should work up to 16TB (or 8TB without your
> fixes) without even requiring larger ffs block sizes than the default of
> 16K. (I used 8K instead of 16K in this example because ffs has problems
> reading the superblock starting at 8K.)
Unfortunatly the fdisk label then becomes a problem for compatible
machines. The standard for ia64 machines looks like being hte way to go
and we already have tools a code to handle it..
>
> > [...]
> > > - int v, lineno = 0, errors = 0;
> > > + unsigned int v;
>
> Style bug: "unsigned int" is spelled u_int everywhere else in disklabel.c
is it compulsary? "unsigned int" is standard C.
"u_int" is used in exactly one place in disklabel.c where
"unsigned" occured in many so to maintain style I should change the one
u_int to "unsigned int".
I'm happy to change to u_int though. (it's shorter)
> except in one place (which shouldn't even use an unsigned type).
where?
>
> Badly chosen type: 'v' is general purpose; it should have type u_long if
> anything, though it wants to be uint32_t for sector counts.
do you mind if I don't rewrite the whole program?
'v' works so I'll leave it.
The aim is not to fix every "bde" inthe program but to be able to
partition a 2TB drive.
>
> > > + int lineno = 0, errors = 0;
> > > int i;
> > [...]
> > > - v = atoi(tp);
> > > + v = strtoul(tp, NULL, 0);
>
> This code is almost as low-quality as before. The following errors are
> not checked for:
"almost" but not worse.. and now it works, for the criteria I'm
looking for (2TB drives)..
> - garbage input
> - overflow inside strtoul()
> - overflow on assignment of the result of strtoul() to v (since we chose
> a poor type for v). The old poorly chosen type was at least suitable
> for holding the result of atoi().
The new one is suitable for strtoul() which is alll that is used now.
>
> > > if ((unsigned)v >= DKMAXTYPES)
> > > fprintf(stderr, "line %d:%s %d\n", lineno,
> > > "Warning, unknown disk type", v);
> >
> > This cast is redundant since v is already unsigned. The fprintf() format
> > string needs to use %u instead of %d.
>
> But v really wants to be a plain int here.
why? all teh types are +ve and lp->d_type is unsigned.
>
> > Use 10 as the base argument to
> > stroul(), not 0 otherwise numbers with leading zeros will be interpreted
> > as octal.
done
(see next email for new diff)
>
> > [...]
> > > - v = atoi(tp);
> > > + v = strtoul(tp, NULL, 0);
> > > if (v <= 0 || (v % DEV_BSIZE) != 0) {
> > > fprintf(stderr,
> > > "line %d: %s: bad sector size\n",
> >
> > Should be == 0, not <= 0 since v is unsigned.
> >
> > [...]
> > > - v = atoi(tp);
> > > + v = strtoul(tp, NULL, 0);
> > > if (v < 0) {
> > > fprintf(stderr, "line %d: %s: bad %s\n",
> > > lineno, tp, cp);
> >
> > v < 0 is impossible.
>
> These v's really want to be plain ints too. The old code was correct for
> all these v's, modulo the usual sloppy parsing giving by atoi().
>
> The sloppy parsing shouldn't be fixed now. Just use a different variable
> and different parsing for the sector counts.
sector counts can not be -ve so it should be unsigned..
I may substitute a u_int16_t variable for some.
>
> Bruce
>
>
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0211161329240.53326-100000>