[review request] zfsboot/zfsloader: support accessing filesystems within a pool

[Andriy Gapon]

on 07/05/2012 16:53 John Baldwin said the following:
> On Saturday, May 05, 2012 5:53:07 am Andriy Gapon wrote:
[snip]
>> The new patchset: http://people.freebsd.org/~avg/zfsboot.patches.7.diff
> 
> Looks great, thanks!  A few replies below:

Here's a followup patch for the suggestions:
http://people.freebsd.org/~avg/bootargs.followup.diff
I will merge it into the main patch.

What do you think about the -LOCORE- change that Bruce inspired?

>>>> - 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?

Done in the above patch.

> 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.

Will do.

> 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).

The genassym approach sounds good, but, indeed - later :)

Thank you.

-- 
Andriy Gapon