Date: Mon, 07 May 2012 17:47:05 +0300 From: Andriy Gapon <avg@FreeBSD.org> To: John Baldwin <jhb@FreeBSD.org> Cc: freebsd-fs@FreeBSD.org, freebsd-hackers@FreeBSD.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool Message-ID: <4FA7E069.8020208@FreeBSD.org> In-Reply-To: <201205070953.04032.jhb@freebsd.org> References: <4F8999D2.1080902@FreeBSD.org> <4FA4F36A.6030903@FreeBSD.org> <4FA4F883.2060008@FreeBSD.org> <201205070953.04032.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4FA7E069.8020208>