Re: [Qemu-devel] [PATCH] VMDK: add monolithic flat image support

[Kevin Wolf]

Am 30.05.2011 09:49, schrieb Fam Zheng:
> VMDK multiple file images can not be recognized for now. This patch is
> adding monolithic flat support to it, that is the image type with two
> files, one text descriptor file and a plain data file. This type of
> image can be created in VMWare, with the options "allocate all disk
> space now" and "store virtual disk as a single file" checked.
> 
> A VmdkExtent structure is introduced to hold the image "extent"
> information, which makes further adding multi extents support of VMDK
> easy. An image creating option "flat" is added for creating flat
> (preallocated) image.
> 
> Signed-off-by: Feiran (Fam) Zheng <address@hidden>

Ok, seems I commented on so many details that in the end I forgot to add
the general comment. :-)

I think this patch is too big to be well reviewable. You should always
try to make only one logical change in one patch. I think you can split
this at least in two parts: First adding the VmdkExtent data structure
without adding any new functionality, and second adding the monolithic
flat support. Depending on how big the second patch is, you can split it
further into image creation and the rest (or any other split that you
feel is natural).

On two or three hunks I commented that they probably aren't required for
monolithic flat support. They should be separate bugfix or cleanup patches.

Kevin