Re: [Qemu-devel] [PATCH 2/6] VMDK: add twoGbMaxExtentSparse support

[Stefan Hajnoczi]

On Wed, Jul 27, 2011 at 10:27 AM, Fam Zheng <address@hidden> wrote:
> Add twoGbMaxExtentSparse support. Only opening code is changed.
>
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block/vmdk.c |  124 
> ++++++++++++++++++++++++++++++++++++----------------------
>  1 files changed, 77 insertions(+), 47 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 9e6c67a..9d1ae32 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -357,7 +357,9 @@ static int vmdk_init_tables(BlockDriverState *bs, 
> VmdkExtent *extent)
>     return ret;
>  }
>
> -static int vmdk_open_vmdk3(BlockDriverState *bs, int flags)
> +static int vmdk_open_vmdk3(BlockDriverState *bs,
> +                           BlockDriverState *file,
> +                           int flags)
>  {
>     int ret;
>     uint32_t magic;
> @@ -365,10 +367,9 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int 
> flags)
>     BDRVVmdkState *s = bs->opaque;
>     VmdkExtent *extent;
>
> -    s->desc_offset = 0x200;
> -    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
> +    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>     if (ret < 0) {
> -        goto fail;
> +        return ret;
>     }
>     extent = vmdk_add_extent(bs,
>                              bs->file, false,
> @@ -378,17 +379,16 @@ static int vmdk_open_vmdk3(BlockDriverState *bs, int 
> flags)
>                              le32_to_cpu(header.granularity));
>     ret = vmdk_init_tables(bs, extent);
>     if (ret) {
> -        /* vmdk_init_tables cleans up on fail, so only free allocation of
> -         * vmdk_add_extent here. */
> -        goto fail;
> +        /* free extent allocated by vmdk_add_extent */
> +        s->num_extents--;
> +        qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));

The return value of qemu_realloc() is the new address of s->extents[].
 You are ignoring the new address and continue to use the old one.

Please add a vmdk_free_last_extent() function that encapsulates this.
The rest of the vmdk.c code should not manipulate
s->extents[]/s->num_extents.  Use vmdk_add_extent(),
vmdk_free_last_extent(), vmdk_free_extents() instead of spreading the
extent management code in various places.

>     }
> -    return 0;
> - fail:
> -    vmdk_free_extents(bs);
>     return ret;
>  }
>
> -static int vmdk_open_vmdk4(BlockDriverState *bs, int flags)
> +static int vmdk_open_vmdk4(BlockDriverState *bs,
> +                           BlockDriverState *file,
> +                           int flags)
>  {
>     int ret;
>     uint32_t magic;
> @@ -397,39 +397,30 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, int 
> flags)
>     BDRVVmdkState *s = bs->opaque;
>     VmdkExtent *extent;
>
> -    s->desc_offset = 0x200;
> -    ret = bdrv_pread(bs->file, sizeof(magic), &header, sizeof(header));
> +    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>     if (ret < 0) {
> -        goto fail;
> +        return ret;
>     }
>     l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
>                         * le64_to_cpu(header.granularity);
> +    if (l1_entry_sectors <= 0) {
> +        return -EINVAL;
> +    }
>     l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
>                 / l1_entry_sectors;
> -    extent = vmdk_add_extent(bs, bs->file, false,
> +    extent = vmdk_add_extent(bs, file, false,
>                           le64_to_cpu(header.capacity),
>                           le64_to_cpu(header.gd_offset) << 9,
>                           le64_to_cpu(header.rgd_offset) << 9,
>                           l1_size,
>                           le32_to_cpu(header.num_gtes_per_gte),
>                           le64_to_cpu(header.granularity));
> -    if (extent->l1_entry_sectors <= 0) {
> -        ret = -EINVAL;
> -        goto fail;
> -    }
> -    /* try to open parent images, if exist */
> -    ret = vmdk_parent_open(bs);
> -    if (ret) {
> -        goto fail;
> -    }
> -    s->parent_cid = vmdk_read_cid(bs, 1);
>     ret = vmdk_init_tables(bs, extent);
>     if (ret) {
> -        goto fail;
> +        /* free extent allocated by vmdk_add_extent */
> +        s->num_extents--;
> +        qemu_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));

Same bug.

>     }
> -    return 0;
> - fail:
> -    vmdk_free_extents(bs);
>     return ret;
>  }
>
> @@ -460,6 +451,35 @@ static int vmdk_parse_description(const char *desc, 
> const char *opt_name,
>     return 0;
>  }
>
> +/* Open an extent file and append to bs array */
> +static int vmdk_open_sparse(BlockDriverState *bs,
> +                            BlockDriverState *file,
> +                            int flags)
> +{
> +    int ret;
> +    uint32_t magic;
> +
> +    if (bdrv_pread(file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
> +        return -EIO;
> +    }
> +
> +    magic = be32_to_cpu(magic);
> +    if (magic == VMDK3_MAGIC) {
> +        ret = vmdk_open_vmdk3(bs, file, flags);
> +        if (ret) {
> +            return ret;
> +        }
> +    } else if (magic == VMDK4_MAGIC) {
> +        ret = vmdk_open_vmdk4(bs, file, flags);
> +        if (ret) {
> +            return ret;
> +        }
> +    } else {
> +        return -EINVAL;
> +    }
> +    return 0;

Shorter way of writing this (ret is no longer necessary):

switch (magic) {
case VMDK3_MAGIC:
    return vmdk_open_vmdk3(bs, file, flags);
case VMDK4_MAGIC:
    return vmdk_open_vmdk4(bs, file, flags);
default:
    return -EINVAL;
}

> +}
> +
>  static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>         const char *desc_file_path)
>  {
> @@ -470,6 +490,8 @@ static int vmdk_parse_extents(const char *desc, 
> BlockDriverState *bs,
>     const char *p = desc;
>     int64_t sectors = 0;
>     int64_t flat_offset;
> +    char extent_path[PATH_MAX];
> +    BlockDriverState *extent_file;
>
>     while (*p) {
>         /* parse extent line:
> @@ -504,22 +526,28 @@ static int vmdk_parse_extents(const char *desc, 
> BlockDriverState *bs,
>             goto next_line;
>         }
>
> +        path_combine(extent_path, sizeof(extent_path),
> +                desc_file_path, fname);
> +        ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
> +        if (ret) {
> +            return ret;
> +        }
> +
>         /* save to extents array */
>         if (!strcmp(type, "FLAT")) {
>             /* FLAT extent */
> -            char extent_path[PATH_MAX];
> -            BlockDriverState *extent_file;
>             VmdkExtent *extent;
>
> -            path_combine(extent_path, sizeof(extent_path),
> -                    desc_file_path, fname);
> -            ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
> -            if (ret) {
> -                return ret;
> -            }
>             extent = vmdk_add_extent(bs, extent_file, true, sectors,
>                             0, 0, 0, 0, sectors);
>             extent->flat_start_offset = flat_offset;
> +        } else if (!strcmp(type, "SPARSE")) {
> +            /* SPARSE extent */
> +            ret = vmdk_open_sparse(bs, extent_file, bs->open_flags);
> +            if (ret) {
> +                bdrv_delete(extent_file);
> +                return ret;
> +            }
>         } else {
>             /* SPARSE extent, not supported for now */

This comment is outdated.

>             fprintf(stderr,
> @@ -552,6 +580,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int 
> flags)
>         return -EINVAL;
>     }
>     if (strcmp(ct, "monolithicFlat") &&
> +        strcmp(ct, "twoGbMaxExtentSparse") &&
>         strcmp(ct, "twoGbMaxExtentFlat")) {
>         fprintf(stderr,
>                 "VMDK: Not supported image type \"%s\""".\n", ct);
> @@ -574,17 +603,18 @@ static int vmdk_open_desc_file(BlockDriverState *bs, 
> int flags)
>
>  static int vmdk_open(BlockDriverState *bs, int flags)
>  {
> -    uint32_t magic;
> -
> -    if (bdrv_pread(bs->file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
> -        return -EIO;
> -    }
> +    int ret;
> +    BDRVVmdkState *s = bs->opaque;
>
> -    magic = be32_to_cpu(magic);
> -    if (magic == VMDK3_MAGIC) {
> -        return vmdk_open_vmdk3(bs, flags);
> -    } else if (magic == VMDK4_MAGIC) {
> -        return vmdk_open_vmdk4(bs, flags);
> +    if (vmdk_open_sparse(bs, bs->file, flags) == 0) {
> +        s->desc_offset = 0x200;
> +        /* try to open parent images, if exist */
> +        ret = vmdk_parent_open(bs);
> +        if (ret) {
> +            return ret;

What needs to be freed here?

Stefan