From owner-freebsd-hackers@FreeBSD.ORG Mon May 7 14:47:09 2012 Return-Path: Delivered-To: freebsd-hackers@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D8898106567A; Mon, 7 May 2012 14:47:09 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id C12CE8FC14; Mon, 7 May 2012 14:47:08 +0000 (UTC) Received: from odyssey.starpoint.kiev.ua (alpha-e.starpoint.kiev.ua [212.40.38.101]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id RAA06503; Mon, 07 May 2012 17:47:07 +0300 (EEST) (envelope-from avg@FreeBSD.org) Message-ID: <4FA7E069.8020208@FreeBSD.org> Date: Mon, 07 May 2012 17:47:05 +0300 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:12.0) Gecko/20120503 Thunderbird/12.0.1 MIME-Version: 1.0 To: John Baldwin References: <4F8999D2.1080902@FreeBSD.org> <4FA4F36A.6030903@FreeBSD.org> <4FA4F883.2060008@FreeBSD.org> <201205070953.04032.jhb@freebsd.org> In-Reply-To: <201205070953.04032.jhb@freebsd.org> X-Enigmail-Version: 1.5pre Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: freebsd-fs@FreeBSD.org, freebsd-hackers@FreeBSD.org, Bruce Evans 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: Mon, 07 May 2012 14:47:10 -0000 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