From owner-cvs-all Tue Dec 17 19: 1:52 2002 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 1903637B401; Tue, 17 Dec 2002 19:01:48 -0800 (PST) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id 697D443EB2; Tue, 17 Dec 2002 19:01:46 -0800 (PST) (envelope-from bde@zeta.org.au) Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id OAA29900; Wed, 18 Dec 2002 14:01:40 +1100 Date: Wed, 18 Dec 2002 14:02:43 +1100 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: "Brian F. Feldman" Cc: "M. Warner Losh" , , Subject: Re: cvs commit: src/sys/boot/i386/boot2 boot2.c In-Reply-To: <200212172347.gBHNlNiP014104@green.bikeshed.org> Message-ID: <20021218130326.G23022-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-cvs-all@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On Tue, 17 Dec 2002, Brian F. Feldman wrote: > "M. Warner Losh" wrote: > > In message: <200212172213.gBHMD7wd013565@green.bikeshed.org> > > "Brian F. Feldman" writes: > > : Warner Losh wrote: > > : > imp 2002/12/17 14:00:06 PST > > : > > > : > Modified files: > > : > sys/boot/i386/boot2 boot2.c > > : > Log: > > : > Reduce diffs with Peter's expanded diffs: > > : > 1) Put back the keyboard printing printf, at the cost of 58 bytes. > > : > 2) Minor tweak to getstr at no apparent cost. > > : > > : Hmm, after all this work on boot2, I remain confused as to why noone else > > : working on the boot blocks seems to have any trouble reinstalling them > > : to test. Am I really the only one that had to fix disklabel.c so that it > > : would install boot blocks instead of just dying on a post-GEOM system? > > : It seems strange that it's only a few people here that would be having that > > : issue :) > > > > I had to hack disklabel.c in unnatural ways to install to test. > > Is this better? This seems to be mainly to work around missing support for DIOCWLABEL in GEOM. I don't use GEOM, so I have no problems with disk labels. > ==== //depot/vendor/freebsd/src/sbin/disklabel/disklabel.c#26 (text+ko) - //depot/projects/trustedbsd/mac/sbin/disklabel/disklabel.c#15 (text+ko) ==== content > ... > @@ -142,7 +148,7 @@ > > char namebuf[BBSIZE], *np = namebuf; > struct disklabel lab; > -char bootarea[BBSIZE]; > +char bootarea[MAXBBSIZE]; This should be dynamically allocated so that there is no arbitrary limit. > @@ -233,7 +239,6 @@ > argv += optind; > #if NUMBOOT > 0 > if (installboot) { > - rflag++; This seems to break the -B -w case (-B historically implied -r when combined with -w, although it shouldn't have; for -B alone, setting rflag is just a kludge to permit overwriting the label when we don't really want to). > ... > @@ -467,15 +473,41 @@ > cksum ^= *sp1++; > sl->sl_cksum = cksum; > #endif > - /* > - * write enable label sector before write (if necessary), > - * disable after writing. > - */ > - flag = 1; > - (void)ioctl(f, DIOCWLABEL, &flag); > - if (write(f, boot, lp->d_bbsize) != (int)lp->d_bbsize) { > - warn("write"); > - return (1); > + if (op != WRITEBOOT) { I think this shouldn't be a special case. Just always write the boot blocks except for the label sector one sector at a time in the -B case like your changes do, and write the label sector directly only in the -rw case. I think the -rw case is currently broken by the missing DIOCWLABEL support, but this should't affect writing of the boot blocks except (unfortunately) with the latest hacks to the boot blocks which steal part of the label sector for boot code. Did you have problems before these hacks? > + /* > + * write enable label sector before write (if > + * necessary), disable after writing. > + */ Style bugs (from original code): various English errors (non-capitalized not-quite-sentence and comma splice). > + flag = 1; > + (void)ioctl(f, DIOCWLABEL, &flag); > + if (write(f, boot, lp->d_bbsize) != (int)lp->d_bbsize) { Style bug: line too long. Indenting everything messed up the formatting. > + warn("write"); > + return (1); > + } > + } else { > + /* > + * Write out all of the boot area except > + * for the sector reserved for the disklabel > + * itself; that part is written only by > + * the kernel, and we can't get it right. > + */ Non-GEOM kernels can handle this almost right. > + ssize_t labelareabegin, labelareaend; Style bug: nested declaration. > + > + labelareabegin = (LABELSECTOR * lp->d_secsize) > + + LABELOFFSET; This won't work unless LABELOFFSET is 0, since block devices have been axed so only writes of (regions of) full sectors can work. Style bugs: (1) excessive parenthesization. Multiplication has precedence over addition in any reasonable language and normal style depends on this. (2) operator not at end of line. Fixing (1) would make room for it there. > + labelareaend = labelareabegin + lp->d_secsize; The latest hacks to the boot blocks break the end being on a sector boundary too. > + if (write(f, boot, labelareabegin) != > + labelareabegin) { > + warn("write"); > + return (1); > + } > + (void)lseek(f, (off_t)labelareaend, SEEK_SET); > + if (write(f, boot + labelareaend, > + lp->d_bbsize - labelareaend) != > + lp->d_bbsize - labelareaend) { > + warn("write"); > + return (1); > + } I think the latest hacks to the boot blocks break this split-up. There is now boot code in the label sector so the above doesn't write all of the boot code, so it can only work if the boot code in the label sector was written previously using some magic and hasn't changed. I don't quite understand how the (op != WRITEBOOT) case works. Doesn't this case occur for -Brw as well as for -rw? If it works for -rw then whatever magic it uses to write to the label sector for labels should work for writing there for boot blocks too. The magic used to be simply the DIOCWLABEL. > ... > @@ -1092,6 +1130,16 @@ > lp->d_secperunit = v; > continue; > } > + if (streq(cp, "boot block size")) { > + v = strtoul(tp, NULL, 10); > + if (v == 0 || v > UINT_MAX) { UINT_MAX is the wrong limit here (unless u_int's happen to be 32 bits). > + fprintf(stderr, "line %d: %s: bad %s\n", > + lineno, tp, cp); > + errors++; > + } else > + lp->d_bbsize = v; > + continue; > + } My version of disklabel.c has bounds checking for d_bbsize in checklabel() but not here. Several fields seem to be checked in both checklabel() and when they are read from an ASCII label; perhaps the checks should be combined. Bruce To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message