Date: Fri, 4 May 2012 11:25:14 -0400 From: John Baldwin <jhb@freebsd.org> To: Andriy Gapon <avg@freebsd.org> Cc: freebsd-fs@freebsd.org, freebsd-hackers@freebsd.org, Gavin Mu <gavin.mu@gmail.com>, Marius Strobl <marius@alchemy.franken.de> Subject: Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool Message-ID: <201205041125.15155.jhb@freebsd.org> In-Reply-To: <4FA2A307.2090108@FreeBSD.org> References: <4F8999D2.1080902@FreeBSD.org> <4FA29E1B.7040005@FreeBSD.org> <4FA2A307.2090108@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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: - Please move the type of the 'kargs' struct into the new header and give it a type name. Don't add the LOADER_ZFS_SUPPORT #ifdef's however or the new size field (see below). - Add #ifndef LOCORE guards to the new header around the structure so it can be used in assembly as well as C. - Move BI_SIZE and ARGOFF into the header as constants. - Add a CTASSERT() in loader/main.c that BI_SIZE == sizeof(struct bootinfo) - Add a KA_SIZE for sizeof(struct kargs) with a CTASSERT. Use it in this instruction: + addl 0x1c(%esp),%ecx # Add size of variable args in place of the 0x1c - Don't add the new 'size' field to the end of the 'struct kargs'. Instead add a comment saying that if KARGS_FLAGS_EXT is set, then a structure will be present at (kargs + 1) that has a int size as its first member. Maybe create a 'struct kargs_ext' that looks like this: struct kargs_ext { uint32_t size; char data[0]; }; - Use 'zargs = (struct zfs_boot_args)(kargs + 1)' in loader/main.c. - Change the ARGADJ line in btxcsu to be this: .set ARGADJ,0x1000 - ARGOFF - If you are adventurous, you could even add a new constant ARGSPACE or some such with an appropriate comment in the new header that you use to replace the 0x1000 in btx.S and in the definition of ARGADJ. Hope that isn't too much, but I do think this will help make things even more understandable. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201205041125.15155.jhb>