From owner-freebsd-hackers@FreeBSD.ORG Fri May 4 15:35:00 2012 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id C70C71065672; Fri, 4 May 2012 15:35:00 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id 92E898FC16; Fri, 4 May 2012 15:35:00 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id F138AB948; Fri, 4 May 2012 11:34:59 -0400 (EDT) From: John Baldwin To: Andriy Gapon Date: Fri, 4 May 2012 11:25:14 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p13; KDE/4.5.5; amd64; ; ) References: <4F8999D2.1080902@FreeBSD.org> <4FA29E1B.7040005@FreeBSD.org> <4FA2A307.2090108@FreeBSD.org> In-Reply-To: <4FA2A307.2090108@FreeBSD.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201205041125.15155.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Fri, 04 May 2012 11:35:00 -0400 (EDT) Cc: freebsd-fs@freebsd.org, freebsd-hackers@freebsd.org, Gavin Mu , Marius Strobl Subject: Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 May 2012 15:35:00 -0000 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