From owner-freebsd-current Sat Nov 16 13:45:12 2002 Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 55CA237B401; Sat, 16 Nov 2002 13:45:09 -0800 (PST) Received: from sccrmhc02.attbi.com (sccrmhc02.attbi.com [204.127.202.62]) by mx1.FreeBSD.org (Postfix) with ESMTP id 9BF1443E77; Sat, 16 Nov 2002 13:45:08 -0800 (PST) (envelope-from julian@elischer.org) Received: from InterJet.elischer.org ([12.232.168.4]) by sccrmhc02.attbi.com (InterMail vM.4.01.03.27 201-229-121-127-20010626) with ESMTP id <20021116214506.MGLI21905.sccrmhc02.attbi.com@InterJet.elischer.org>; Sat, 16 Nov 2002 21:45:06 +0000 Received: from localhost (localhost.elischer.org [127.0.0.1]) by InterJet.elischer.org (8.9.1a/8.9.1) with ESMTP id NAA55862; Sat, 16 Nov 2002 13:44:48 -0800 (PST) Date: Sat, 16 Nov 2002 13:44:47 -0800 (PST) From: Julian Elischer To: Bruce Evans Cc: Tim Robbins , FreeBSD current users Subject: Re: Sign fixes for disklabel(8) In-Reply-To: <20021116194439.E17742-100000@gamplex.bde.org> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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