Re: [Qemu-devel] [PATCH 16/20] vmdk: push vmdk_read_desc up to caller

[Fam Zheng]

On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Currently, we just try reading a VMDK file as both image and descriptor.
> This makes it hard to choose which of the two attempts gave the best error.
> We'll decide in advance if the file looks like an image or a descriptor,
> and this patch is the first step to that end.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  block/vmdk.c | 54 ++++++++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 58f4c34..74a0bac 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -526,8 +526,8 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
>      return ret;
>  }
>  
> -static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
> -                               uint64_t desc_offset, Error **errp);
> +static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
> +                               Error **errp);
>  
>  static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
>                              Error **errp)
> @@ -575,7 +575,13 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
>      if (header.capacity == 0) {
>          uint64_t desc_offset = le64_to_cpu(header.desc_offset);
>          if (desc_offset) {
> -            return vmdk_open_desc_file(bs, flags, desc_offset << 9, errp);
> +            char *buf = vmdk_read_desc(file, desc_offset << 9, errp);
> +            if (!buf) {
> +                return -EINVAL;
> +            }
> +            ret = vmdk_open_desc_file(bs, flags, buf, errp);
> +            g_free(buf);
> +            return ret;
>          }
>      }
>  
> @@ -726,16 +732,12 @@ static int vmdk_parse_description(const char *desc, 
> const char *opt_name,
>  
>  /* Open an extent file and append to bs array */
>  static int vmdk_open_sparse(BlockDriverState *bs,
> -                            BlockDriverState *file,
> -                            int flags, Error **errp)
> +                            BlockDriverState *file, int flags,
> +                            char *buf, Error **errp)
>  {
>      uint32_t magic;
>  
> -    if (bdrv_pread(file, 0, &magic, sizeof(magic)) != sizeof(magic)) {
> -        return -EIO;
> -    }
> -
> -    magic = be32_to_cpu(magic);
> +    magic = ldl_be_p(buf);
>      switch (magic) {
>          case VMDK3_MAGIC:
>              return vmdk_open_vmfs_sparse(bs, file, flags, errp);
> @@ -819,7 +821,12 @@ static int vmdk_parse_extents(const char *desc, 
> BlockDriverState *bs,
>              extent->flat_start_offset = flat_offset << 9;
>          } else if (!strcmp(type, "SPARSE") || !strcmp(type, "VMFSSPARSE")) {
>              /* SPARSE extent and VMFSSPARSE extent are both "COWD" sparse 
> file*/
> -            ret = vmdk_open_sparse(bs, extent_file, bs->open_flags, errp);
> +            char *buf = vmdk_read_desc(extent_file, 0, errp);
> +            if (!buf) {
> +                ret = -EINVAL;
> +            } else {
> +                ret = vmdk_open_sparse(bs, extent_file, bs->open_flags, buf, 
> errp);
> +            }
>              if (ret) {

Is buf leaked here when vmdk_open_sparse fails?

Fam

>                  bdrv_unref(extent_file);
>                  return ret;