Re: [Qemu-devel] [PATCH v8 00/12] Adding VMDK monolithic flat support

[Stefan Hajnoczi]

On Tue, Jul 5, 2011 at 12:31 PM, Fam Zheng <address@hidden> wrote:
> Chnages from v7:
>    03/12: remove deadloop in probing descriptor file.
>
> Fam Zheng (12):
>  VMDK: introduce VmdkExtent
>  VMDK: bugfix, align offset to cluster in get_whole_cluster
>  VMDK: probe for monolithicFlat images
>  VMDK: separate vmdk_open by format version
>  VMDK: add field BDRVVmdkState.desc_offset
>  VMDK: flush multiple extents
>  VMDK: move 'static' cid_update flag to bs field
>  VMDK: change get_cluster_offset return type
>  VMDK: open/read/write for monolithicFlat image
>  VMDK: create different subformats
>  VMDK: fix coding style
>  block: add bdrv_get_allocated_file_size() operation
>
>  block.c           |   19 +
>  block.h           |    1 +
>  block/raw-posix.c |   21 +
>  block/raw-win32.c |   29 ++
>  block/vmdk.c      | 1361 
> +++++++++++++++++++++++++++++++++++++----------------
>  block_int.h       |    2 +
>  qemu-img.c        |   31 +--
>  7 files changed, 1024 insertions(+), 440 deletions(-)

Getting closer.  Patch 10/12 is big and I see a lot of repetition in
the image creation process.  I wonder if it helps to factor out
certain aspects such as filename and descriptor generation so that the
creation function doesn't become so large.  The aim is to encapsulate
aspects of image creation cleanly so that the caller doesn't need to
keep state around and can use a simple interface to orchestrate image
creation.

Structuring is going to be important for future vmdk changes.  We need
to introduce clean interfaces to separate subformats while keeping
common code shared as utility functions.  Right now there is a lot of
if (flat) { ... } or if (!strcmp(..., "monolithicSparse")) { ... }.
The details of various formats are spread throughout the entire vmdk
codebase instead of encapsulated in one module each.  This will become
even more important as you flesh out the support matrix for various
file formats.  Something to keep in mind for future work after this
series.

Stefan