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

[Fam Zheng]

On 2013年12月02日 21:26, Stefan Hajnoczi wrote:
> 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 <address@hidden>
> > ---
> >   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?

Yes it does. The limit is far above this, there is a statement in VMDK 
spec: "Maximum VMDK file size is 2TB." It opens this file from the test 
case, however I can't generate such a twoGbMaxExtentFlat from 
Workstation, because it will be automatically forced to monolithicFlat 
(if the size is above certain threshold).

> 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.

Just that I'd like to keep it consistent that exit point is either 
direct return statement or goto jump, in one function, so it will be 
easy to reason the code path.

> > @@ -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);

OK, good idea. Thanks

Fam