From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MWyAn-0005fG-Mn for qemu-devel@nongnu.org; Fri, 31 Jul 2009 15:53:49 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MWyAi-0005dj-QO for qemu-devel@nongnu.org; Fri, 31 Jul 2009 15:53:49 -0400 Received: from [199.232.76.173] (port=58663 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MWyAi-0005dg-LB for qemu-devel@nongnu.org; Fri, 31 Jul 2009 15:53:44 -0400 Received: from moutng.kundenserver.de ([212.227.17.10]:56339) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MWyAh-0004Pz-9C for qemu-devel@nongnu.org; Fri, 31 Jul 2009 15:53:44 -0400 Message-ID: <4A734BC5.7070300@mail.berlios.de> Date: Fri, 31 Jul 2009 21:53:41 +0200 From: Stefan Weil MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Add new block driver for the VDI format (use aio) References: <4A6888AC.3050509@mail.berlios.de> <1248380985-7362-1-git-send-email-weil@mail.berlios.de> <4A697C7E.80400@redhat.com> <4A69DF48.7000109@mail.berlios.de> <4A6D5EA2.2080706@redhat.com> <20090731150449.GB11631@lst.de> In-Reply-To: <20090731150449.GB11631@lst.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christoph Hellwig Cc: Kevin Wolf , QEMU Developers Christoph Hellwig schrieb: > On Mon, Jul 27, 2009 at 10:00:34AM +0200, Kevin Wolf wrote: > >> Ok, that makes sense. Probably we should remove the #define completely >> then. I mean, why creating images that nobody - not even we ourselves - >> can read? >> > > I agree. As mentioned during the previous rounds all these ifdef parts > of code that can only be compiled in/out by touching the source code are > really bad. Either they are good enough to be enabled unconditionally > (or at least through configure if they require a library or similar) or > they are broken / useless enough to not bother. If virtualbox only > supports 1k block size images and we do aswell there's no point in > carrying around this dead code. > > >>>> I guess you should remove this block before the patch is included. >>>> >>>> >>> This is also one of the details were hints of the block driver experts >>> would be helpful as I did not understand this aio_remove / aio_cancel >>> mechanism. >>> >> I wouldn't consider myself an AIO expert and I don't want to tell you >> something wrong, so maybe Christoph would be the right one here? >> > > #if 0 is a horrible way for hints. Coments with XXX: or TODO: are much > better documentation. I'll take a look at the aio implementation, but > I'm far from expert on the qemu aio code. > > >>> I think it would be possible to extend the specification >>> to support compression or encryption. >>> >>> The official specification (as far as I know it) does not >>> support compression (nor encryption). >>> >> Then remove the entry. The function vdi_write_compressed doesn't exist >> and doesn't even make sense with the current specification. The same >> applies for vdi_set_key. >> > > Seconded, keeping function stubs around just bloats and obsfucates the > code without reason. > > > Hi Christoph Thanks for the review. Most of your comments were considered in my latest patch version which I just sent to the list. I assume that there will be a need for block sizes larger than 1 MiB in very large images, so I did not remove these parts of the code. Regards Stefan