Date: Wed, 18 Dec 2002 14:02:43 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: "Brian F. Feldman" <green@FreeBSD.org> Cc: "M. Warner Losh" <imp@bsdimp.com>, <cvs-committers@FreeBSD.org>, <cvs-all@FreeBSD.org> Subject: Re: cvs commit: src/sys/boot/i386/boot2 boot2.c Message-ID: <20021218130326.G23022-100000@gamplex.bde.org> In-Reply-To: <200212172347.gBHNlNiP014104@green.bikeshed.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 17 Dec 2002, Brian F. Feldman wrote:
> "M. Warner Losh" <imp@bsdimp.com> wrote:
> > In message: <200212172213.gBHMD7wd013565@green.bikeshed.org>
> > "Brian F. Feldman" <green@FreeBSD.org> writes:
> > : Warner Losh <imp@FreeBSD.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20021218130326.G23022-100000>