Re: [Qemu-devel] [PATCH] allow reading variable size vmdk descriptor files

[Stefan Hajnoczi <stefanha@redhat.com>]

On Wed, Jun 12, 2013 at 11:04:44AM +0300, Evgeny Budilovsky wrote:
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 608daaf..1bc944b 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -719,27 +719,41 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
>                                 int64_t desc_offset)
>  {
>      int ret;
> -    char buf[2048];
> +    char *buf = NULL;
>      char ct[128];
>      BDRVVmdkState *s = bs->opaque;
> +    int64_t size;
> 
> -    ret = bdrv_pread(bs->file, desc_offset, buf, sizeof(buf));
> +    size = bdrv_get_allocated_file_size(bs);

Please use bdrv_getlength() instead of bdrv_get_allocated_file_size(),
which checks stat.st_blocks.  From the stat(2) man page:

  The st_blocks field indicates the number of blocks  allocated  to
  the  file,  512-byte  units.  (This may be smaller than st_size/512
  when the file has holes.).

> +    if (size < 0) {
> +        return -EINVAL;
> +    }
> +
> +    buf = g_malloc0(size+1);

Spaces please:
g_malloc0(size + 1)

> +
> +    ret = bdrv_pread(bs->file, desc_offset, buf, size);
>      if (ret < 0) {
> -        return ret;
> +        goto exit;
>      }
> -    buf[2047] = '\0';

The buffer must be NUL-terminated.

>      if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
> -        return -EMEDIUMTYPE;
> +        ret = -EMEDIUMTYPE;
> +        goto exit;
>      }
>      if (strcmp(ct, "monolithicFlat") &&
>          strcmp(ct, "twoGbMaxExtentSparse") &&
>          strcmp(ct, "twoGbMaxExtentFlat")) {
>          fprintf(stderr,
>                  "VMDK: Not supported image type \"%s\""".\n", ct);
> -        return -ENOTSUP;
> +        ret = -ENOTSUP;
> +        goto exit;
>      }
>      s->desc_offset = 0;
> -    return vmdk_parse_extents(buf, bs, bs->file->filename);
> +    ret = vmdk_parse_extents(buf, bs, bs->file->filename);
> +exit:
> +    if (buf) {
> +        g_free(buf);
> +    }

The if is not necessary since g_free(NULL) is a nop.