Re: [Qemu-devel] [PATCH] vmdk: Fix creating big description file

[Stefan Hajnoczi <stefanha@redhat.com>]

On Mon, Dec 02, 2013 at 11:01:20AM +0800, Fam Zheng wrote:
> The buffer for description file was 4096 which only covers a few
> hundred of extents. This changes the buffer to dynamic allocated with
> g_strdup_printf in order to support bigger cases.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/vmdk.c               |   65 +-
>  tests/qemu-iotests/059     |    5 +
>  tests/qemu-iotests/059.out | 2012 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 2058 insertions(+), 24 deletions(-)

Does VMware have a hard limit?  For example, does it open the 1000 GB
twoGbMaxExtentFlat file from your test case?

Minor comments below but feel free to keep the code as-is if you wish:

> @@ -1625,7 +1625,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>          "ddb.adapterType = \"%s\"\n";
>  
>      if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
> -        return -EINVAL;
> +        ret = -EINVAL;
> +        goto exit;
>      }
>      /* Read out options */
>      while (options && options->name) {
> @@ -1651,7 +1652,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>                 strcmp(adapter_type, "lsilogic") &&
>                 strcmp(adapter_type, "legacyESX")) {
>          error_setg(errp, "Unknown adapter type: '%s'", adapter_type);
> -        return -EINVAL;
> +        ret = -EINVAL;
> +        goto exit;
>      }
>      if (strcmp(adapter_type, "ide") != 0) {
>          /* that's the number of heads with which vmware operates when
> @@ -1667,7 +1669,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>                 strcmp(fmt, "twoGbMaxExtentFlat") &&
>                 strcmp(fmt, "streamOptimized")) {
>          error_setg(errp, "Unknown subformat: '%s'", fmt);
> -        return -EINVAL;
> +        ret = -EINVAL;
> +        goto exit;
>      }
>      split = !(strcmp(fmt, "twoGbMaxExtentFlat") &&
>                strcmp(fmt, "twoGbMaxExtentSparse"));
> @@ -1681,22 +1684,25 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>      }
>      if (flat && backing_file) {
>          error_setg(errp, "Flat image can't have backing file");
> -        return -ENOTSUP;
> +        ret = -ENOTSUP;
> +        goto exit;
>      }
>      if (flat && zeroed_grain) {
>          error_setg(errp, "Flat image can't enable zeroed grain");
> -        return -ENOTSUP;
> +        ret = -ENOTSUP;
> +        goto exit;
>      }
>      if (backing_file) {
>          BlockDriverState *bs = bdrv_new("");
>          ret = bdrv_open(bs, backing_file, NULL, 0, NULL, errp);
>          if (ret != 0) {
>              bdrv_unref(bs);
> -            return ret;
> +            goto exit;
>          }
>          if (strcmp(bs->drv->format_name, "vmdk")) {
>              bdrv_unref(bs);
> -            return -EINVAL;
> +            ret = -EINVAL;
> +            goto exit;
>          }
>          parent_cid = vmdk_read_cid(bs, 0);
>          bdrv_unref(bs);

Changing these to goto isn't really necessary.

> @@ -1730,25 +1737,31 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>  
>          if (vmdk_create_extent(ext_filename, size,
>                                 flat, compress, zeroed_grain)) {
> -            return -EINVAL;
> +            ret = -EINVAL;
> +            goto exit;
>          }
>          filesize -= size;
>  
>          /* Format description line */
>          snprintf(desc_line, sizeof(desc_line),
>                      desc_extent_line, size / 512, desc_filename);
> -        pstrcat(ext_desc_lines, sizeof(ext_desc_lines), desc_line);
> +        new_desc_lines = g_strdup_printf("%s%s",
> +                                         ext_desc_lines ? : "",
> +                                         desc_line);
> +        g_free(ext_desc_lines);
> +        ext_desc_lines = new_desc_lines;

These lines can be eliminated if you use GString:

/* Format description line */
g_string_append_printf(ext_desc_lines,
                       desc_extent_line,
		       size / 512,
		       desc_filename);