Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 7 May 2012 09:53:03 -0400
From:      John Baldwin <jhb@freebsd.org>
To:        freebsd-fs@freebsd.org
Cc:        freebsd-hackers@freebsd.org, Andriy Gapon <avg@freebsd.org>
Subject:   Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool
Message-ID:  <201205070953.04032.jhb@freebsd.org>
In-Reply-To: <4FA4F883.2060008@FreeBSD.org>
References:  <4F8999D2.1080902@FreeBSD.org> <4FA4F36A.6030903@FreeBSD.org> <4FA4F883.2060008@FreeBSD.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Saturday, May 05, 2012 5:53:07 am Andriy Gapon wrote:
> on 05/05/2012 12:31 Andriy Gapon said the following:
> > on 04/05/2012 18:25 John Baldwin said the following:
> >> On Thursday, May 03, 2012 11:23:51 am Andriy Gapon wrote:
> >>> on 03/05/2012 18:02 Andriy Gapon said the following:
> >>>>
> >>>> Here's the latest version of the patches:
> >>>> http://people.freebsd.org/~avg/zfsboot.patches.4.diff
> >>>
> >>> I've found a couple of problems in the previous version, so here's another one:
> >>> http://people.freebsd.org/~avg/zfsboot.patches.5.diff
> >>> The important change is in the first patch (__exec args).
> >>
> >> A few comments/suggestions on the args bits:
> > 
> > John,
> > 
> > these are excellent suggestions!  Thank you!
> 
> The new patchset: http://people.freebsd.org/~avg/zfsboot.patches.7.diff

Looks great, thanks!  A few replies below:

> >> - Add a CTASSERT() in loader/main.c that BI_SIZE == sizeof(struct bootinfo)
> > 
> > I have added a definition of CTASSERT to boostrap.h as it was not available for
> > sys/boot and there were two local definitions of the macro in individual files.
> > 
> > However the assertion would fail right now.
> > The backward-compatible value of BI_SIZE (72 == 0x48) covers only part of the
> > fields in struct bootinfo, those up to the following comment:
> > 	/* Items below only from advanced bootloader */
> > 
> > I am a little bit hesitant: should I increase BI_SIZE to cover the whole struct
> > bootinfo or should I compare BI_SIZE to offsetof bi_kernend?

Actually, we should probably be reading the 'bi_size' field and not using a BI_SIZE
constant at all?

Looks like only the non-functional EFI boot loader doesn't set bi_size (and it should
just be fixed to do so since it needs to pass new fields in anyway).

> > I've decided to define ARGADJ in the new common header, then I've had to rename
> > btxcsu.s to .S, so that the preprocessing is executed for it.

Ok.  Maybe add one comment to the bootargs.h head to explain that the 'bootargs'
struct starts at ARGOFF and can grow up, while struct bootinfo is copied such that
it's end is at the top of the argument area and grows down.

Also, at some point we could use a genassym.c file ala the kernel builds to generate
some of the constants in bootargs.h instead (e.g. the offsets of fields within
structures, and BA_SIZE, though we probably want to ensure that BA_SIZE never
changes).

-- 
John Baldwin



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201205070953.04032.jhb>