Re: [Qemu-devel] [PATCH] Add new block driver for the VDI format (use aio)

[Stefan Weil <weil@mail.berlios.de>]

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