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 _______________________________________________ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
