From owner-freebsd-geom@FreeBSD.ORG Sun Mar 16 18:50:57 2008 Return-Path: Delivered-To: freebsd-geom@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 6B8D61065676 for ; Sun, 16 Mar 2008 18:50:57 +0000 (UTC) (envelope-from rick@kiwi-computer.com) Received: from kiwi-computer.com (keira.kiwi-computer.com [63.224.10.3]) by mx1.freebsd.org (Postfix) with SMTP id 0CFDB8FC26 for ; Sun, 16 Mar 2008 18:50:56 +0000 (UTC) (envelope-from rick@kiwi-computer.com) Received: (qmail 65298 invoked by uid 2001); 16 Mar 2008 18:50:55 -0000 Date: Sun, 16 Mar 2008 12:50:55 -0600 From: "Rick C. Petty" To: freebsd-geom@freebsd.org Message-ID: <20080316185055.GA64920@keira.kiwi-computer.com> References: <20080310052711.GA49676@keira.kiwi-computer.com> <20080313153551.82wlu8iio4088c44@webmail.ntnu.no> <20080313182257.GB14969@keira.kiwi-computer.com> <20080316090554.GA1230@carrot.studby.ntnu.no> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20080316090554.GA1230@carrot.studby.ntnu.no> User-Agent: Mutt/1.4.2.3i Subject: Re: [patch] geom_vinum platform fixes X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: rick-freebsd@kiwi-computer.com List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 16 Mar 2008 18:50:57 -0000 On Sun, Mar 16, 2008 at 10:05:55AM +0100, Ulf Lilleengen wrote: > > I've reviewed the patch and done some modifications to it. I'll need some > testing first though (I don't have a testbed right now since I'm travelling). I've reviewed your patch but haven't had time to test it yet (hopefully this week..) > Here are some notes: > - Removed GV_VERSION. It is not used anywhere. Well, the point of this macro is for future changes to vinum on-disk structure, since I added version as part of the new magic. Strictly speaking, the macro isn't necessary (yet) and perhaps no future changes will happen to the structures. > - Avoid moving header includes around. Not sure I agree. Although style(9) doesn't specify that the sys/* headers should be alphabetized, many other places in kernel code have them sorted as such (with certain exceptions like sys/param). It was a trivial "fix" I had hoped to sneak in since that file was being modified anyway. > - Declare functions static when used inside one file. > - Style fixes. *nod* > - In gv_legacy_header_type, you can't be guaranteed that the hdr fields is > not always null (although I agree in practise its highly unlikely), but > it's hard to check this any other way, so I think it's alright :) I can't think of any _better_ way to check. In the struct timeval fields on amd64, I can't think of how these "zeros" would not appear. The bytes past the end of the i386 header will always be zero because of the malloc with M_ZERO. > - Use macro values instead of hard-coded values when testing legacy type. > - Pass data variable as argument rather than returning a header. > - Avoid memory allocation where possible. All good! > Please let me know how it fares, and thanks for your help btw :) I will do some testing using your patch. And thank _you_ for reviewing this. I really look forward to your upcoming merge from p4! -- Rick C. Petty