[3/5] vmdk: Implement .bdrv_co_create callback
diff mbox series

Message ID 20180509055802.28423-4-famz@redhat.com
State New
Headers show
Series
  • vmdk: Implement x-blockdev-create
Related show

Commit Message

Fam Zheng May 9, 2018, 5:58 a.m. UTC
This makes VMDK support x-blockdev-create. The implementation reuses the
image creation code in vmdk_co_create_opts which now acceptes a callback
pointer to "retrieve" BlockBackend pointers from the caller. This way we
separate the logic between file/extent acquisition and initialization.

The QAPI command parameters are mostly the same as the old create_opts
except the dropped legacy @compat6 switch, which is redundant with
@hwversion.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c         | 481 +++++++++++++++++++++++++++++++++++++--------------
 qapi/block-core.json |  67 ++++++-
 2 files changed, 418 insertions(+), 130 deletions(-)

Comments

Markus Armbruster May 9, 2018, 12:41 p.m. UTC | #1
Beware, I'm only looking at QAPI-related aspects.

Fam Zheng <famz@redhat.com> writes:

> This makes VMDK support x-blockdev-create. The implementation reuses the
> image creation code in vmdk_co_create_opts which now acceptes a callback
> pointer to "retrieve" BlockBackend pointers from the caller. This way we
> separate the logic between file/extent acquisition and initialization.
>
> The QAPI command parameters are mostly the same as the old create_opts
> except the dropped legacy @compat6 switch, which is redundant with
> @hwversion.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/vmdk.c         | 481 +++++++++++++++++++++++++++++++++++++--------------
>  qapi/block-core.json |  67 ++++++-
>  2 files changed, 418 insertions(+), 130 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 083942f806..e16b04e26a 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1905,38 +1905,87 @@ static int filename_decompose(const char *filename, char *path, char *prefix,
>      return VMDK_OK;
>  }
>  
> -static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
> -                                            Error **errp)
> +/* Stringify BlockdevVmdkSubformat enum in the faimiliar camelCase. */
> +static const char *vmdk_subformat_str(BlockdevVmdkSubformat subformat)
>  {
> -    int idx = 0;
> -    BlockBackend *new_blk = NULL;
> +    switch (subformat) {
> +    case BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE:
> +        return "monolithicSparse";
> +    case BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICFLAT:
> +        return "monolithicFlat";
> +    case BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT:
> +        return "twoGbMaxExtentFlat";
> +    case BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTSPARSE:
> +        return "twoGbMaxExtentSparse";
> +    case BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED:
> +        return "streamOptimized";
> +    default:
> +        abort();
> +    }
> +}

I'd use an array instead of a switch.

The standard mapping from enum FOO to string is FOO_lookup[], commonly
used via qapi_enum_lookup().

vmdk_subformat_str() differs from BlockdevVmdkSubformat_lookup[] only in
case: the former is CamelCase ("monolithicSparse"), the latter is all
lower case (like "monolithicsparse"), because that's how it's spelled in
the QAPI schema.  By the way, QAPI naming conventions ask for hyphens,
like "monolithic-sparse".

Why do you need CamelCase?  Is it for an existing external interface?

If yes, should we use CamelCase in the schema?

Should we use hyphens, and have this function map hyphen followed by
lower case letter to upper case letter?

> +
> +/*
> + * idx == 0: get or create the descriptor file (also the image file if in a
> + *           non-split format.
> + * idx >= 1: get the n-th extent if in a split subformat
> + */
> +typedef BlockBackend *(*vmdk_create_extent_fn)(int64_t size,
> +                                               int idx,
> +                                               bool flat,
> +                                               bool split,
> +                                               bool compress,
> +                                               bool zeroed_grain,
> +                                               void *opaque,
> +                                               Error **errp);
> +
> +static void vmdk_desc_add_extent(GString *desc,
> +                                 const char *extent_line_fmt,
> +                                 int64_t size, const char *filename)
> +{
> +    char *desc_line = g_malloc0(BUF_SIZE);
> +    const char *basename = strrchr(filename, '/');

Blank line between declarations and statements, if you don't mind.

> +    if (!basename) {
> +        basename = filename;
> +    } else {
> +        basename += 1;
> +    }

g_path_get_basename()?

> +    snprintf(desc_line, BUF_SIZE, extent_line_fmt,
> +             DIV_ROUND_UP(size, BDRV_SECTOR_SIZE),
> +             basename);
> +    g_string_append(desc, desc_line);
> +    g_free(desc_line);

g_string_append_printf()?

> +}
> +
> +static int coroutine_fn vmdk_co_do_create(int64_t size,
> +                                          BlockdevVmdkSubformat subformat,
> +                                          BlockdevVmdkAdapterType adapter_type,
> +                                          const char *backing_file,
> +                                          const char *hw_version,
> +                                          bool compat6,
> +                                          bool zeroed_grain,
> +                                          vmdk_create_extent_fn extent_fn,
> +                                          void *opaque,
> +                                          Error **errp)
> +{
> +    int extent_idx;
> +    BlockBackend *blk;
>      Error *local_err = NULL;
>      char *desc = NULL;
> -    int64_t total_size = 0, filesize;
> -    char *adapter_type = NULL;
> -    char *backing_file = NULL;
> -    char *hw_version = NULL;
> -    char *fmt = NULL;
>      int ret = 0;
>      bool flat, split, compress;
>      GString *ext_desc_lines;
> -    char *path = g_malloc0(PATH_MAX);
> -    char *prefix = g_malloc0(PATH_MAX);
> -    char *postfix = g_malloc0(PATH_MAX);
> -    char *desc_line = g_malloc0(BUF_SIZE);
> -    char *ext_filename = g_malloc0(PATH_MAX);
> -    char *desc_filename = g_malloc0(PATH_MAX);
>      const int64_t split_size = 0x80000000;  /* VMDK has constant split size */
> -    const char *desc_extent_line;
> +    int64_t extent_size;
> +    int64_t created_size = 0;
> +    const char *extent_line_fmt;
>      char *parent_desc_line = g_malloc0(BUF_SIZE);
>      uint32_t parent_cid = 0xffffffff;
>      uint32_t number_heads = 16;
> -    bool zeroed_grain = false;
>      uint32_t desc_offset = 0, desc_len;
>      const char desc_template[] =
>          "# Disk DescriptorFile\n"
>          "version=1\n"
> -        "CID=%" PRIx32 "\n"
> +        "CID=%08" PRIx32 "\n"

How is this change related to the patch's purpose?

>          "parentCID=%" PRIx32 "\n"
>          "createType=\"%s\"\n"
>          "%s"
> @@ -1955,71 +2004,35 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
>  
>      ext_desc_lines = g_string_new(NULL);
>  
> -    if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
> -        ret = -EINVAL;
> -        goto exit;
> -    }
>      /* Read out options */
> -    total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> -                          BDRV_SECTOR_SIZE);
> -    adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
> -    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> -    hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION);
> -    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false)) {
> -        if (strcmp(hw_version, "undefined")) {
> +    if (compat6) {
> +        if (hw_version) {
>              error_setg(errp,
>                         "compat6 cannot be enabled with hwversion set");
>              ret = -EINVAL;
>              goto exit;
>          }
> -        g_free(hw_version);
> -        hw_version = g_strdup("6");
> +        hw_version = "6";
>      }
> -    if (strcmp(hw_version, "undefined") == 0) {
> -        g_free(hw_version);
> -        hw_version = g_strdup("4");
> -    }
> -    fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
> -    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false)) {
> -        zeroed_grain = true;
> +    if (!hw_version) {
> +        hw_version = "4";
>      }
>  
> -    if (!adapter_type) {
> -        adapter_type = g_strdup("ide");
> -    } else if (strcmp(adapter_type, "ide") &&
> -               strcmp(adapter_type, "buslogic") &&
> -               strcmp(adapter_type, "lsilogic") &&
> -               strcmp(adapter_type, "legacyESX")) {
> -        error_setg(errp, "Unknown adapter type: '%s'", adapter_type);
> -        ret = -EINVAL;
> -        goto exit;
> -    }
> -    if (strcmp(adapter_type, "ide") != 0) {
> +    if (adapter_type != BLOCKDEV_VMDK_ADAPTER_TYPE_IDE) {
>          /* that's the number of heads with which vmware operates when
>             creating, exporting, etc. vmdk files with a non-ide adapter type */
>          number_heads = 255;
>      }
> -    if (!fmt) {
> -        /* Default format to monolithicSparse */
> -        fmt = g_strdup("monolithicSparse");
> -    } else if (strcmp(fmt, "monolithicFlat") &&
> -               strcmp(fmt, "monolithicSparse") &&
> -               strcmp(fmt, "twoGbMaxExtentSparse") &&
> -               strcmp(fmt, "twoGbMaxExtentFlat") &&
> -               strcmp(fmt, "streamOptimized")) {
> -        error_setg(errp, "Unknown subformat: '%s'", fmt);
> -        ret = -EINVAL;
> -        goto exit;
> -    }
> -    split = !(strcmp(fmt, "twoGbMaxExtentFlat") &&
> -              strcmp(fmt, "twoGbMaxExtentSparse"));
> -    flat = !(strcmp(fmt, "monolithicFlat") &&
> -             strcmp(fmt, "twoGbMaxExtentFlat"));
> -    compress = !strcmp(fmt, "streamOptimized");
> +    split = (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT) ||
> +            (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTSPARSE);
> +    flat = (subformat == BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICFLAT) ||
> +           (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT);
> +    compress = subformat == BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED;
> +
>      if (flat) {
> -        desc_extent_line = "RW %" PRId64 " FLAT \"%s\" 0\n";
> +        extent_line_fmt = "RW %" PRId64 " FLAT \"%s\" 0\n";
>      } else {
> -        desc_extent_line = "RW %" PRId64 " SPARSE \"%s\"\n";
> +        extent_line_fmt = "RW %" PRId64 " SPARSE \"%s\"\n";
>      }
>      if (flat && backing_file) {
>          error_setg(errp, "Flat image can't have backing file");
> @@ -2031,10 +2044,34 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
>          ret = -ENOTSUP;
>          goto exit;
>      }
> +
> +    /* Create extents */
> +    if (split) {
> +        extent_size = split_size;
> +    } else {
> +        extent_size = size;
> +    }
> +    if (!split && !flat) {
> +        created_size = extent_size;
> +    } else {
> +        created_size = 0;
> +    }
> +    /* Get the descriptor file BDS */
> +    blk = extent_fn(created_size, 0, flat, split, compress, zeroed_grain,
> +                    opaque, errp);
> +    if (!blk) {
> +        ret = -EIO;
> +        goto exit;
> +    }
> +    if (!split && !flat) {
> +        vmdk_desc_add_extent(ext_desc_lines, extent_line_fmt, created_size,
> +                             blk_bs(blk)->filename);
> +    }
> +
>      if (backing_file) {
> -        BlockBackend *blk;
> +        BlockBackend *backing;
>          char *full_backing = g_new0(char, PATH_MAX);
> -        bdrv_get_full_backing_filename_from_filename(filename, backing_file,
> +        bdrv_get_full_backing_filename_from_filename(blk_bs(blk)->filename, backing_file,
>                                                       full_backing, PATH_MAX,
>                                                       &local_err);
>          if (local_err) {
> @@ -2044,93 +2081,65 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
>              goto exit;
>          }
>  
> -        blk = blk_new_open(full_backing, NULL, NULL,
> -                           BDRV_O_NO_BACKING, errp);
> +        backing = blk_new_open(full_backing, NULL, NULL,
> +                               BDRV_O_NO_BACKING, errp);
>          g_free(full_backing);
> -        if (blk == NULL) {
> +        if (backing == NULL) {
>              ret = -EIO;
>              goto exit;
>          }
> -        if (strcmp(blk_bs(blk)->drv->format_name, "vmdk")) {
> -            blk_unref(blk);
> +        if (strcmp(blk_bs(backing)->drv->format_name, "vmdk")) {
> +            error_setg(errp, "Invalid backing file format: %s. Must be vmdk",
> +                       blk_bs(backing)->drv->format_name);
> +            blk_unref(backing);
>              ret = -EINVAL;
>              goto exit;
>          }
> -        ret = vmdk_read_cid(blk_bs(blk), 0, &parent_cid);
> -        blk_unref(blk);
> +        ret = vmdk_read_cid(blk_bs(backing), 0, &parent_cid);
> +        blk_unref(backing);
>          if (ret) {
> +            error_setg(errp, "Failed to read parent CID");
>              goto exit;
>          }
>          snprintf(parent_desc_line, BUF_SIZE,
>                  "parentFileNameHint=\"%s\"", backing_file);
>      }
> -
> -    /* Create extents */
> -    filesize = total_size;
> -    while (filesize > 0) {
> -        int64_t size = filesize;
> -
> -        if (split && size > split_size) {
> -            size = split_size;
> -        }
> -        if (split) {
> -            snprintf(desc_filename, PATH_MAX, "%s-%c%03d%s",
> -                    prefix, flat ? 'f' : 's', ++idx, postfix);
> -        } else if (flat) {
> -            snprintf(desc_filename, PATH_MAX, "%s-flat%s", prefix, postfix);
> -        } else {
> -            snprintf(desc_filename, PATH_MAX, "%s%s", prefix, postfix);
> -        }
> -        snprintf(ext_filename, PATH_MAX, "%s%s", path, desc_filename);
> -
> -        if (vmdk_create_extent(ext_filename, size,
> -                               flat, compress, zeroed_grain, NULL, opts, errp)) {
> +    extent_idx = 1;
> +    while (created_size < size) {
> +        BlockBackend *extent_blk;
> +        int64_t cur_size = MIN(size - created_size, extent_size);
> +        extent_blk = extent_fn(cur_size, extent_idx, flat, split, compress,
> +                               zeroed_grain, opaque, errp);
> +        if (!extent_blk) {
>              ret = -EINVAL;
>              goto exit;
>          }
> -        filesize -= size;
> -
> -        /* Format description line */
> -        snprintf(desc_line, BUF_SIZE,
> -                    desc_extent_line, size / BDRV_SECTOR_SIZE, desc_filename);
> -        g_string_append(ext_desc_lines, desc_line);
> +        vmdk_desc_add_extent(ext_desc_lines, extent_line_fmt, cur_size,
> +                             blk_bs(extent_blk)->filename);
> +        created_size += cur_size;
> +        extent_idx++;
> +        blk_unref(extent_blk);
>      }
>      /* generate descriptor file */
>      desc = g_strdup_printf(desc_template,
>                             g_random_int(),
>                             parent_cid,
> -                           fmt,
> +                           vmdk_subformat_str(subformat),
>                             parent_desc_line,
>                             ext_desc_lines->str,
>                             hw_version,
> -                           total_size /
> +                           size /
>                                 (int64_t)(63 * number_heads * BDRV_SECTOR_SIZE),
>                             number_heads,
> -                           adapter_type);
> +                           qapi_enum_lookup(&BlockdevVmdkAdapterType_lookup,
> +                                            adapter_type));
>      desc_len = strlen(desc);
>      /* the descriptor offset = 0x200 */
>      if (!split && !flat) {
>          desc_offset = 0x200;
> -    } else {
> -        ret = bdrv_create_file(filename, opts, &local_err);
> -        if (ret < 0) {
> -            error_propagate(errp, local_err);
> -            goto exit;
> -        }
>      }
>  
> -    new_blk = blk_new_open(filename, NULL, NULL,
> -                           BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> -                           &local_err);
> -    if (new_blk == NULL) {
> -        error_propagate(errp, local_err);
> -        ret = -EIO;
> -        goto exit;
> -    }
> -
> -    blk_set_allow_write_beyond_eof(new_blk, true);
> -
> -    ret = blk_pwrite(new_blk, desc_offset, desc, desc_len, 0);
> +    ret = blk_pwrite(blk, desc_offset, desc, desc_len, 0);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Could not write description");
>          goto exit;
> @@ -2138,12 +2147,148 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
>      /* bdrv_pwrite write padding zeros to align to sector, we don't need that
>       * for description file */
>      if (desc_offset == 0) {
> -        ret = blk_truncate(new_blk, desc_len, PREALLOC_MODE_OFF, errp);
> +        ret = blk_truncate(blk, desc_len, PREALLOC_MODE_OFF, errp);
>      }
>  exit:
> -    if (new_blk) {
> -        blk_unref(new_blk);
> +    if (blk) {
> +        blk_unref(blk);
>      }
> +    g_free(desc);
> +    g_free(parent_desc_line);
> +    g_string_free(ext_desc_lines, true);
> +    return ret;
> +}
> +
> +typedef struct {
> +    char *path;
> +    char *prefix;
> +    char *postfix;
> +    QemuOpts *opts;
> +} VMDKCreateOptsData;
> +
> +static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
> +                                            bool flat, bool split, bool compress,
> +                                            bool zeroed_grain, void *opaque,
> +                                            Error **errp)
> +{
> +    BlockBackend *blk = NULL;
> +    BlockDriverState *bs = NULL;
> +    VMDKCreateOptsData *data = opaque;
> +    char *ext_filename = NULL;
> +    char *rel_filename = NULL;
> +
> +    if (idx == 0) {
> +        rel_filename = g_strdup_printf("%s%s", data->prefix, data->postfix);
> +    } else if (split) {
> +        rel_filename = g_strdup_printf("%s-%c%03d%s",
> +                                       data->prefix,
> +                                       flat ? 'f' : 's', idx, data->postfix);
> +    } else {
> +        assert(idx == 1);
> +        rel_filename = g_strdup_printf("%s-flat%s", data->prefix, data->postfix);
> +    }
> +
> +    ext_filename = g_strdup_printf("%s%s", data->path, rel_filename);
> +    g_free(rel_filename);
> +
> +    if (vmdk_create_extent(ext_filename, size,
> +                           flat, compress, zeroed_grain, &blk, data->opts,
> +                           errp)) {
> +        goto exit;
> +    }
> +    bdrv_unref(bs);
> +exit:
> +    g_free(ext_filename);
> +    return blk;
> +}
> +
> +static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
> +                                            Error **errp)
> +{
> +    Error *local_err = NULL;
> +    char *desc = NULL;
> +    int64_t total_size = 0;
> +    char *adapter_type = NULL;
> +    BlockdevVmdkAdapterType adapter_type_enum;
> +    char *backing_file = NULL;
> +    char *hw_version = NULL;
> +    char *fmt = NULL;
> +    BlockdevVmdkSubformat subformat;
> +    int ret = 0;
> +    char *path = g_malloc0(PATH_MAX);
> +    char *prefix = g_malloc0(PATH_MAX);
> +    char *postfix = g_malloc0(PATH_MAX);
> +    char *desc_line = g_malloc0(BUF_SIZE);
> +    char *ext_filename = g_malloc0(PATH_MAX);
> +    char *desc_filename = g_malloc0(PATH_MAX);
> +    char *parent_desc_line = g_malloc0(BUF_SIZE);
> +    bool zeroed_grain;
> +    bool compat6;
> +    int i;
> +    VMDKCreateOptsData data;
> +
> +    if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
> +        ret = -EINVAL;
> +        goto exit;
> +    }
> +    /* Read out options */
> +    total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> +                          BDRV_SECTOR_SIZE);
> +    adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
> +    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> +    hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION);
> +    compat6 = qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false);
> +    if (strcmp(hw_version, "undefined") == 0) {
> +        g_free(hw_version);
> +        hw_version = g_strdup("4");
> +    }
> +    fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
> +    zeroed_grain = qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false);
> +
> +    if (adapter_type) {
> +        for (i = 0; i < strlen(adapter_type); ++i) {
> +            adapter_type[i] = qemu_tolower(adapter_type[i]);
> +        }

First, you convert to lower cases, and then...

> +        adapter_type_enum = qapi_enum_parse_full(&BlockdevVmdkAdapterType_lookup,
> +                                                 adapter_type,
> +                                                 BLOCKDEV_VMDK_ADAPTER_TYPE_IDE,
> +                                                 true,
> +                                                 &local_err);

... you parse case-insensitive.  Huh?

Which spellings did the old code accept?  As far as I can tell, exactly
"ide", "lsilogic", "buslogic", "legacyESX".  Are you sure we should
ignore case going forward?

> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            ret = -EINVAL;
> +            goto exit;
> +        }
> +    } else {
> +        adapter_type_enum = BLOCKDEV_VMDK_ADAPTER_TYPE_IDE;
> +    }
> +
> +    if (!fmt) {
> +        /* Default format to monolithicSparse */
> +        subformat = BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE;
> +    } else {
> +        subformat = qapi_enum_parse_full(&BlockdevVmdkSubformat_lookup,
> +                                         fmt,
> +                                         BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE,
> +                                         true,
> +                                         &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            ret = -EINVAL;
> +            goto exit;
> +        }

Likewise: should we ignore case going forward?  The old code appears to
accept exactly "monolithicFlat", "monolithicSparse",
"twoGbMaxExtentSparse", "twoGbMaxExtentFlat", "streamOptimized".

> +    }
> +    data = (VMDKCreateOptsData){
> +        .prefix = prefix,
> +        .postfix = postfix,
> +        .path = path,
> +        .opts = opts,
> +    };
> +    ret = vmdk_co_do_create(total_size, subformat, adapter_type_enum,
> +                            backing_file, hw_version, compat6, zeroed_grain,
> +                            vmdk_co_create_opts_cb, &data, errp);
> +
> +exit:
>      g_free(adapter_type);
>      g_free(backing_file);
>      g_free(hw_version);
> @@ -2156,7 +2301,84 @@ exit:
>      g_free(ext_filename);
>      g_free(desc_filename);
>      g_free(parent_desc_line);
> -    g_string_free(ext_desc_lines, true);
> +    return ret;
> +}
> +
> +static BlockBackend *vmdk_co_create_cb(int64_t size, int idx,
> +                                       bool flat, bool split, bool compress,
> +                                       bool zeroed_grain, void *opaque,
> +                                       Error **errp)
> +{
> +    int ret;
> +    BlockDriverState *bs;
> +    BlockBackend *blk;
> +    BlockdevCreateOptionsVmdk *opts = opaque;
> +
> +    if (idx == 0) {
> +        bs = bdrv_open_blockdev_ref(opts->file, errp);
> +    } else {
> +        int i;
> +        BlockdevRefList *list = opts->extents;
> +        for (i = 1; i < idx; i++) {
> +            if (!list || !list->next) {
> +                error_setg(errp, "Extent [%d] not specified", i);
> +                return NULL;
> +            }
> +            list = list->next;
> +        }
> +        if (!list) {
> +            error_setg(errp, "Extent [%d] not specified", idx - 1);
> +            return NULL;
> +        }
> +        bs = bdrv_open_blockdev_ref(list->value, errp);
> +    }
> +    if (!bs) {
> +        return NULL;
> +    }
> +    blk = blk_new(BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE,
> +                  BLK_PERM_ALL);
> +    if (blk_insert_bs(blk, bs, errp)) {
> +        bdrv_unref(bs);
> +        return NULL;
> +    }
> +    blk_set_allow_write_beyond_eof(blk, true);
> +    bdrv_unref(bs);
> +
> +    ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain, errp);
> +    if (ret) {
> +        blk_unref(blk);
> +        blk = NULL;
> +    }
> +    return blk;
> +}
> +
> +static int coroutine_fn vmdk_co_create(BlockdevCreateOptions *create_options,
> +                                       Error **errp)
> +{
> +    int ret;
> +    BlockdevCreateOptionsVmdk *opts;
> +
> +    opts = &create_options->u.vmdk;
> +
> +    /* Validate options */
> +    if (!QEMU_IS_ALIGNED(opts->size, BDRV_SECTOR_SIZE)) {
> +        error_setg(errp, "Image size must be a multiple of 512 bytes");
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    ret = vmdk_co_do_create(opts->size,
> +                            opts->subformat,
> +                            opts->adapter_type,
> +                            opts->backing_file,
> +                            opts->hwversion,
> +                            false,
> +                            opts->zeroed_grain,
> +                            vmdk_co_create_cb,
> +                            opts, errp);
> +    return ret;
> +
> +out:
>      return ret;
>  }
>  
> @@ -2424,6 +2646,7 @@ static BlockDriver bdrv_vmdk = {
>      .bdrv_co_pwrite_zeroes        = vmdk_co_pwrite_zeroes,
>      .bdrv_close                   = vmdk_close,
>      .bdrv_co_create_opts          = vmdk_co_create_opts,
> +    .bdrv_co_create               = vmdk_co_create,
>      .bdrv_co_flush_to_disk        = vmdk_co_flush,
>      .bdrv_co_block_status         = vmdk_co_block_status,
>      .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c50517bff3..df3903b54d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3855,6 +3855,71 @@
>              'size':             'size',
>              '*cluster-size' :   'size' } }
>  
> +##
> +# @BlockdevVmdkSubformat:
> +#
> +# Subformat options for VMDK images
> +#
> +# @monolithicsparse: Single file image with sparse cluster allocation
> +# @monolithicflat: Single flat data image and a descriptor file
> +# @twogbmaxextentsparse: Data is split into 2GB (per virtual LBA) sparse extent
> +#                        files, in addition to a descriptor file
> +# @twogbmaxextentflat: Data is split into 2GB (per virtual LBA) flat extent
> +#                        files, in addition to a descriptor file
> +# @streamoptimized: Single file image sparse cluster allocation, optimized for
> +#                   streaming over network.
> +#
> +# Since: 2.13
> +##
> +{ 'enum': 'BlockdevVmdkSubformat',
> +  'data': [ 'monolithicsparse', 'monolithicflat', 'twogbmaxextentsparse',
> +            'twogbmaxextentflat', 'streamoptimized'] }

alllowercasewithoutspacesisevenlesslegiblethanCamelCase.
THERESULTINGCIDENTIFIERSAREALLCAPSWITHOUTSPACESWHICHISEVENWORSE.

QAPI conventions ask for monolithic-sparse, monolithic-flat,
two-gb-max-extent-sparse and so forth.  Results in C enum identifiers
BLOCKDEV_VMDK_SUBFORMAT_MONOLITHIC_SPARSE,
BLOCKDEV_VMDK_SUBFORMAT_MONOLITHIC_FLAT,
BLOCKDEV_VMDK_SUBFORMAT_TWO_GB_MAX_EXTENT_SPARSE, ...

The existing external interface appears to ask for monolithicFlat,
monolithicSparse, twoGbMaxExtentSparse, ...  What's the best way to map
between these guys and a QAPI enum?

> +
> +##
> +# @BlockdevVmdkAdapterType:
> +#
> +# Adapter type info for VMDK images
> +#
> +# Since: 2.13
> +##
> +{ 'enum': 'BlockdevVmdkAdapterType',
> +  'data': [ 'ide', 'buslogic', 'lsilogic', 'legacyesx'] }
> +
> +##
> +# @BlockdevCreateOptionsVmdk:
> +#
> +# Driver specific image creation options for VMDK.
> +#
> +# @file         Where to store the new image file. This refers to the image
> +#               file for monolithcSparse and streamOptimized format, or the
> +#               descriptor file for other formats.
> +# @size         Size of the virtual disk in bytes
> +# @extents      Where to store the data extents. Required for monolithcflat,
> +#               twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For
> +#               monolithicflat, only one entry is required; for
> +#               twoGbMaxExtent* formats, the number of entries required is
> +#               calculated as extent_number = virtual_size / 2GB.
> +# @subformat    The subformat of the VMDK image. Default: "monolithicsparse".
> +# @backing-file The path of backing file. Default: no backing file is used.
> +# @adapter-type The adapter type used to fill in the descriptor. Default: ide.
> +# @hwversion    Hardware version. The meaningful options are "4" or "6".

Okay, these are the meaningfull options.  What are the meaningless ones?

> +#               Defaulted to "4".

More common phrasings are

    Default is "4"
    Defaults to "4"
    Default: "4"

> +# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats.
> +#               Default: false.
> +#
> +# Since: 2.13
> +##
> +{ 'struct': 'BlockdevCreateOptionsVmdk',
> +  'data': { 'file':             'BlockdevRef',
> +            'size':             'size',
> +            '*extents':          ['BlockdevRef'],
> +            '*subformat':       'BlockdevVmdkSubformat',
> +            '*backing-file':    'str',
> +            '*adapter-type':    'BlockdevVmdkAdapterType',
> +            '*hwversion':       'str',
> +            '*zeroed-grain':    'bool' } }
> +
> +
>  ##
>  # @SheepdogRedundancyType:
>  #
> @@ -4078,7 +4143,7 @@
>        'throttle':       'BlockdevCreateNotSupported',
>        'vdi':            'BlockdevCreateOptionsVdi',
>        'vhdx':           'BlockdevCreateOptionsVhdx',
> -      'vmdk':           'BlockdevCreateNotSupported',
> +      'vmdk':           'BlockdevCreateOptionsVmdk',
>        'vpc':            'BlockdevCreateOptionsVpc',
>        'vvfat':          'BlockdevCreateNotSupported',
>        'vxhs':           'BlockdevCreateNotSupported'
Fam Zheng May 9, 2018, 2:19 p.m. UTC | #2
On Wed, 05/09 14:41, Markus Armbruster wrote:
> Beware, I'm only looking at QAPI-related aspects.
> 
> Fam Zheng <famz@redhat.com> writes:
> 
> > This makes VMDK support x-blockdev-create. The implementation reuses the
> > image creation code in vmdk_co_create_opts which now acceptes a callback
> > pointer to "retrieve" BlockBackend pointers from the caller. This way we
> > separate the logic between file/extent acquisition and initialization.
> >
> > The QAPI command parameters are mostly the same as the old create_opts
> > except the dropped legacy @compat6 switch, which is redundant with
> > @hwversion.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/vmdk.c         | 481 +++++++++++++++++++++++++++++++++++++--------------
> >  qapi/block-core.json |  67 ++++++-
> >  2 files changed, 418 insertions(+), 130 deletions(-)
> >
> > diff --git a/block/vmdk.c b/block/vmdk.c
> > index 083942f806..e16b04e26a 100644
> > --- a/block/vmdk.c
> > +++ b/block/vmdk.c
> > @@ -1905,38 +1905,87 @@ static int filename_decompose(const char *filename, char *path, char *prefix,
> >      return VMDK_OK;
> >  }
> >  
> > -static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
> > -                                            Error **errp)
> > +/* Stringify BlockdevVmdkSubformat enum in the faimiliar camelCase. */
> > +static const char *vmdk_subformat_str(BlockdevVmdkSubformat subformat)
> >  {
> > -    int idx = 0;
> > -    BlockBackend *new_blk = NULL;
> > +    switch (subformat) {
> > +    case BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE:
> > +        return "monolithicSparse";
> > +    case BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICFLAT:
> > +        return "monolithicFlat";
> > +    case BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT:
> > +        return "twoGbMaxExtentFlat";
> > +    case BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTSPARSE:
> > +        return "twoGbMaxExtentSparse";
> > +    case BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED:
> > +        return "streamOptimized";
> > +    default:
> > +        abort();
> > +    }
> > +}
> 
> I'd use an array instead of a switch.
> 
> The standard mapping from enum FOO to string is FOO_lookup[], commonly
> used via qapi_enum_lookup().
> 
> vmdk_subformat_str() differs from BlockdevVmdkSubformat_lookup[] only in
> case: the former is CamelCase ("monolithicSparse"), the latter is all
> lower case (like "monolithicsparse"), because that's how it's spelled in
> the QAPI schema.  By the way, QAPI naming conventions ask for hyphens,
> like "monolithic-sparse".
> 
> Why do you need CamelCase?  Is it for an existing external interface?
> 
> If yes, should we use CamelCase in the schema?
> 
> Should we use hyphens, and have this function map hyphen followed by
> lower case letter to upper case letter?

"streamOptimized" is the exact style as spelled in the VMDK spec:

https://www.vmware.com/support/developer/vddk/vmdk_50_technote.pdf

so I want to stay as close as possible to it. In other words lower case
"monolithicsparse" feels a bit better than hyphens "monolithic-sparse" in the
QAPI interface.

> 
> > +
> > +/*
> > + * idx == 0: get or create the descriptor file (also the image file if in a
> > + *           non-split format.
> > + * idx >= 1: get the n-th extent if in a split subformat
> > + */
> > +typedef BlockBackend *(*vmdk_create_extent_fn)(int64_t size,
> > +                                               int idx,
> > +                                               bool flat,
> > +                                               bool split,
> > +                                               bool compress,
> > +                                               bool zeroed_grain,
> > +                                               void *opaque,
> > +                                               Error **errp);
> > +
> > +static void vmdk_desc_add_extent(GString *desc,
> > +                                 const char *extent_line_fmt,
> > +                                 int64_t size, const char *filename)
> > +{
> > +    char *desc_line = g_malloc0(BUF_SIZE);
> > +    const char *basename = strrchr(filename, '/');
> 
> Blank line between declarations and statements, if you don't mind.

OK!

> 
> > +    if (!basename) {
> > +        basename = filename;
> > +    } else {
> > +        basename += 1;
> > +    }
> 
> g_path_get_basename()?

Good suggestion, thanks!

> 
> > +    snprintf(desc_line, BUF_SIZE, extent_line_fmt,
> > +             DIV_ROUND_UP(size, BDRV_SECTOR_SIZE),
> > +             basename);
> > +    g_string_append(desc, desc_line);
> > +    g_free(desc_line);
> 
> g_string_append_printf()?

Yes!

> 
> > +}
> > +
> > +static int coroutine_fn vmdk_co_do_create(int64_t size,
> > +                                          BlockdevVmdkSubformat subformat,
> > +                                          BlockdevVmdkAdapterType adapter_type,
> > +                                          const char *backing_file,
> > +                                          const char *hw_version,
> > +                                          bool compat6,
> > +                                          bool zeroed_grain,
> > +                                          vmdk_create_extent_fn extent_fn,
> > +                                          void *opaque,
> > +                                          Error **errp)
> > +{
> > +    int extent_idx;
> > +    BlockBackend *blk;
> >      Error *local_err = NULL;
> >      char *desc = NULL;
> > -    int64_t total_size = 0, filesize;
> > -    char *adapter_type = NULL;
> > -    char *backing_file = NULL;
> > -    char *hw_version = NULL;
> > -    char *fmt = NULL;
> >      int ret = 0;
> >      bool flat, split, compress;
> >      GString *ext_desc_lines;
> > -    char *path = g_malloc0(PATH_MAX);
> > -    char *prefix = g_malloc0(PATH_MAX);
> > -    char *postfix = g_malloc0(PATH_MAX);
> > -    char *desc_line = g_malloc0(BUF_SIZE);
> > -    char *ext_filename = g_malloc0(PATH_MAX);
> > -    char *desc_filename = g_malloc0(PATH_MAX);
> >      const int64_t split_size = 0x80000000;  /* VMDK has constant split size */
> > -    const char *desc_extent_line;
> > +    int64_t extent_size;
> > +    int64_t created_size = 0;
> > +    const char *extent_line_fmt;
> >      char *parent_desc_line = g_malloc0(BUF_SIZE);
> >      uint32_t parent_cid = 0xffffffff;
> >      uint32_t number_heads = 16;
> > -    bool zeroed_grain = false;
> >      uint32_t desc_offset = 0, desc_len;
> >      const char desc_template[] =
> >          "# Disk DescriptorFile\n"
> >          "version=1\n"
> > -        "CID=%" PRIx32 "\n"
> > +        "CID=%08" PRIx32 "\n"
> 
> How is this change related to the patch's purpose?

It's unrelated. I forgot to remove it after testing: unifying the CID print
length helps hexdump based compare of monolithicSparse headers.

> 
> >          "parentCID=%" PRIx32 "\n"
> >          "createType=\"%s\"\n"
> >          "%s"
> > @@ -1955,71 +2004,35 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
> >  
> >      ext_desc_lines = g_string_new(NULL);
> >  
> > -    if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
> > -        ret = -EINVAL;
> > -        goto exit;
> > -    }
> >      /* Read out options */
> > -    total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> > -                          BDRV_SECTOR_SIZE);
> > -    adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
> > -    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> > -    hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION);
> > -    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false)) {
> > -        if (strcmp(hw_version, "undefined")) {
> > +    if (compat6) {
> > +        if (hw_version) {
> >              error_setg(errp,
> >                         "compat6 cannot be enabled with hwversion set");
> >              ret = -EINVAL;
> >              goto exit;
> >          }
> > -        g_free(hw_version);
> > -        hw_version = g_strdup("6");
> > +        hw_version = "6";
> >      }
> > -    if (strcmp(hw_version, "undefined") == 0) {
> > -        g_free(hw_version);
> > -        hw_version = g_strdup("4");
> > -    }
> > -    fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
> > -    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false)) {
> > -        zeroed_grain = true;
> > +    if (!hw_version) {
> > +        hw_version = "4";
> >      }
> >  
> > -    if (!adapter_type) {
> > -        adapter_type = g_strdup("ide");
> > -    } else if (strcmp(adapter_type, "ide") &&
> > -               strcmp(adapter_type, "buslogic") &&
> > -               strcmp(adapter_type, "lsilogic") &&
> > -               strcmp(adapter_type, "legacyESX")) {
> > -        error_setg(errp, "Unknown adapter type: '%s'", adapter_type);
> > -        ret = -EINVAL;
> > -        goto exit;
> > -    }
> > -    if (strcmp(adapter_type, "ide") != 0) {
> > +    if (adapter_type != BLOCKDEV_VMDK_ADAPTER_TYPE_IDE) {
> >          /* that's the number of heads with which vmware operates when
> >             creating, exporting, etc. vmdk files with a non-ide adapter type */
> >          number_heads = 255;
> >      }
> > -    if (!fmt) {
> > -        /* Default format to monolithicSparse */
> > -        fmt = g_strdup("monolithicSparse");
> > -    } else if (strcmp(fmt, "monolithicFlat") &&
> > -               strcmp(fmt, "monolithicSparse") &&
> > -               strcmp(fmt, "twoGbMaxExtentSparse") &&
> > -               strcmp(fmt, "twoGbMaxExtentFlat") &&
> > -               strcmp(fmt, "streamOptimized")) {
> > -        error_setg(errp, "Unknown subformat: '%s'", fmt);
> > -        ret = -EINVAL;
> > -        goto exit;
> > -    }
> > -    split = !(strcmp(fmt, "twoGbMaxExtentFlat") &&
> > -              strcmp(fmt, "twoGbMaxExtentSparse"));
> > -    flat = !(strcmp(fmt, "monolithicFlat") &&
> > -             strcmp(fmt, "twoGbMaxExtentFlat"));
> > -    compress = !strcmp(fmt, "streamOptimized");
> > +    split = (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT) ||
> > +            (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTSPARSE);
> > +    flat = (subformat == BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICFLAT) ||
> > +           (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT);
> > +    compress = subformat == BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED;
> > +
> >      if (flat) {
> > -        desc_extent_line = "RW %" PRId64 " FLAT \"%s\" 0\n";
> > +        extent_line_fmt = "RW %" PRId64 " FLAT \"%s\" 0\n";
> >      } else {
> > -        desc_extent_line = "RW %" PRId64 " SPARSE \"%s\"\n";
> > +        extent_line_fmt = "RW %" PRId64 " SPARSE \"%s\"\n";
> >      }
> >      if (flat && backing_file) {
> >          error_setg(errp, "Flat image can't have backing file");
> > @@ -2031,10 +2044,34 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
> >          ret = -ENOTSUP;
> >          goto exit;
> >      }
> > +
> > +    /* Create extents */
> > +    if (split) {
> > +        extent_size = split_size;
> > +    } else {
> > +        extent_size = size;
> > +    }
> > +    if (!split && !flat) {
> > +        created_size = extent_size;
> > +    } else {
> > +        created_size = 0;
> > +    }
> > +    /* Get the descriptor file BDS */
> > +    blk = extent_fn(created_size, 0, flat, split, compress, zeroed_grain,
> > +                    opaque, errp);
> > +    if (!blk) {
> > +        ret = -EIO;
> > +        goto exit;
> > +    }
> > +    if (!split && !flat) {
> > +        vmdk_desc_add_extent(ext_desc_lines, extent_line_fmt, created_size,
> > +                             blk_bs(blk)->filename);
> > +    }
> > +
> >      if (backing_file) {
> > -        BlockBackend *blk;
> > +        BlockBackend *backing;
> >          char *full_backing = g_new0(char, PATH_MAX);
> > -        bdrv_get_full_backing_filename_from_filename(filename, backing_file,
> > +        bdrv_get_full_backing_filename_from_filename(blk_bs(blk)->filename, backing_file,
> >                                                       full_backing, PATH_MAX,
> >                                                       &local_err);
> >          if (local_err) {
> > @@ -2044,93 +2081,65 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
> >              goto exit;
> >          }
> >  
> > -        blk = blk_new_open(full_backing, NULL, NULL,
> > -                           BDRV_O_NO_BACKING, errp);
> > +        backing = blk_new_open(full_backing, NULL, NULL,
> > +                               BDRV_O_NO_BACKING, errp);
> >          g_free(full_backing);
> > -        if (blk == NULL) {
> > +        if (backing == NULL) {
> >              ret = -EIO;
> >              goto exit;
> >          }
> > -        if (strcmp(blk_bs(blk)->drv->format_name, "vmdk")) {
> > -            blk_unref(blk);
> > +        if (strcmp(blk_bs(backing)->drv->format_name, "vmdk")) {
> > +            error_setg(errp, "Invalid backing file format: %s. Must be vmdk",
> > +                       blk_bs(backing)->drv->format_name);
> > +            blk_unref(backing);
> >              ret = -EINVAL;
> >              goto exit;
> >          }
> > -        ret = vmdk_read_cid(blk_bs(blk), 0, &parent_cid);
> > -        blk_unref(blk);
> > +        ret = vmdk_read_cid(blk_bs(backing), 0, &parent_cid);
> > +        blk_unref(backing);
> >          if (ret) {
> > +            error_setg(errp, "Failed to read parent CID");
> >              goto exit;
> >          }
> >          snprintf(parent_desc_line, BUF_SIZE,
> >                  "parentFileNameHint=\"%s\"", backing_file);
> >      }
> > -
> > -    /* Create extents */
> > -    filesize = total_size;
> > -    while (filesize > 0) {
> > -        int64_t size = filesize;
> > -
> > -        if (split && size > split_size) {
> > -            size = split_size;
> > -        }
> > -        if (split) {
> > -            snprintf(desc_filename, PATH_MAX, "%s-%c%03d%s",
> > -                    prefix, flat ? 'f' : 's', ++idx, postfix);
> > -        } else if (flat) {
> > -            snprintf(desc_filename, PATH_MAX, "%s-flat%s", prefix, postfix);
> > -        } else {
> > -            snprintf(desc_filename, PATH_MAX, "%s%s", prefix, postfix);
> > -        }
> > -        snprintf(ext_filename, PATH_MAX, "%s%s", path, desc_filename);
> > -
> > -        if (vmdk_create_extent(ext_filename, size,
> > -                               flat, compress, zeroed_grain, NULL, opts, errp)) {
> > +    extent_idx = 1;
> > +    while (created_size < size) {
> > +        BlockBackend *extent_blk;
> > +        int64_t cur_size = MIN(size - created_size, extent_size);
> > +        extent_blk = extent_fn(cur_size, extent_idx, flat, split, compress,
> > +                               zeroed_grain, opaque, errp);
> > +        if (!extent_blk) {
> >              ret = -EINVAL;
> >              goto exit;
> >          }
> > -        filesize -= size;
> > -
> > -        /* Format description line */
> > -        snprintf(desc_line, BUF_SIZE,
> > -                    desc_extent_line, size / BDRV_SECTOR_SIZE, desc_filename);
> > -        g_string_append(ext_desc_lines, desc_line);
> > +        vmdk_desc_add_extent(ext_desc_lines, extent_line_fmt, cur_size,
> > +                             blk_bs(extent_blk)->filename);
> > +        created_size += cur_size;
> > +        extent_idx++;
> > +        blk_unref(extent_blk);
> >      }
> >      /* generate descriptor file */
> >      desc = g_strdup_printf(desc_template,
> >                             g_random_int(),
> >                             parent_cid,
> > -                           fmt,
> > +                           vmdk_subformat_str(subformat),
> >                             parent_desc_line,
> >                             ext_desc_lines->str,
> >                             hw_version,
> > -                           total_size /
> > +                           size /
> >                                 (int64_t)(63 * number_heads * BDRV_SECTOR_SIZE),
> >                             number_heads,
> > -                           adapter_type);
> > +                           qapi_enum_lookup(&BlockdevVmdkAdapterType_lookup,
> > +                                            adapter_type));
> >      desc_len = strlen(desc);
> >      /* the descriptor offset = 0x200 */
> >      if (!split && !flat) {
> >          desc_offset = 0x200;
> > -    } else {
> > -        ret = bdrv_create_file(filename, opts, &local_err);
> > -        if (ret < 0) {
> > -            error_propagate(errp, local_err);
> > -            goto exit;
> > -        }
> >      }
> >  
> > -    new_blk = blk_new_open(filename, NULL, NULL,
> > -                           BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> > -                           &local_err);
> > -    if (new_blk == NULL) {
> > -        error_propagate(errp, local_err);
> > -        ret = -EIO;
> > -        goto exit;
> > -    }
> > -
> > -    blk_set_allow_write_beyond_eof(new_blk, true);
> > -
> > -    ret = blk_pwrite(new_blk, desc_offset, desc, desc_len, 0);
> > +    ret = blk_pwrite(blk, desc_offset, desc, desc_len, 0);
> >      if (ret < 0) {
> >          error_setg_errno(errp, -ret, "Could not write description");
> >          goto exit;
> > @@ -2138,12 +2147,148 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
> >      /* bdrv_pwrite write padding zeros to align to sector, we don't need that
> >       * for description file */
> >      if (desc_offset == 0) {
> > -        ret = blk_truncate(new_blk, desc_len, PREALLOC_MODE_OFF, errp);
> > +        ret = blk_truncate(blk, desc_len, PREALLOC_MODE_OFF, errp);
> >      }
> >  exit:
> > -    if (new_blk) {
> > -        blk_unref(new_blk);
> > +    if (blk) {
> > +        blk_unref(blk);
> >      }
> > +    g_free(desc);
> > +    g_free(parent_desc_line);
> > +    g_string_free(ext_desc_lines, true);
> > +    return ret;
> > +}
> > +
> > +typedef struct {
> > +    char *path;
> > +    char *prefix;
> > +    char *postfix;
> > +    QemuOpts *opts;
> > +} VMDKCreateOptsData;
> > +
> > +static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
> > +                                            bool flat, bool split, bool compress,
> > +                                            bool zeroed_grain, void *opaque,
> > +                                            Error **errp)
> > +{
> > +    BlockBackend *blk = NULL;
> > +    BlockDriverState *bs = NULL;
> > +    VMDKCreateOptsData *data = opaque;
> > +    char *ext_filename = NULL;
> > +    char *rel_filename = NULL;
> > +
> > +    if (idx == 0) {
> > +        rel_filename = g_strdup_printf("%s%s", data->prefix, data->postfix);
> > +    } else if (split) {
> > +        rel_filename = g_strdup_printf("%s-%c%03d%s",
> > +                                       data->prefix,
> > +                                       flat ? 'f' : 's', idx, data->postfix);
> > +    } else {
> > +        assert(idx == 1);
> > +        rel_filename = g_strdup_printf("%s-flat%s", data->prefix, data->postfix);
> > +    }
> > +
> > +    ext_filename = g_strdup_printf("%s%s", data->path, rel_filename);
> > +    g_free(rel_filename);
> > +
> > +    if (vmdk_create_extent(ext_filename, size,
> > +                           flat, compress, zeroed_grain, &blk, data->opts,
> > +                           errp)) {
> > +        goto exit;
> > +    }
> > +    bdrv_unref(bs);
> > +exit:
> > +    g_free(ext_filename);
> > +    return blk;
> > +}
> > +
> > +static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
> > +                                            Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +    char *desc = NULL;
> > +    int64_t total_size = 0;
> > +    char *adapter_type = NULL;
> > +    BlockdevVmdkAdapterType adapter_type_enum;
> > +    char *backing_file = NULL;
> > +    char *hw_version = NULL;
> > +    char *fmt = NULL;
> > +    BlockdevVmdkSubformat subformat;
> > +    int ret = 0;
> > +    char *path = g_malloc0(PATH_MAX);
> > +    char *prefix = g_malloc0(PATH_MAX);
> > +    char *postfix = g_malloc0(PATH_MAX);
> > +    char *desc_line = g_malloc0(BUF_SIZE);
> > +    char *ext_filename = g_malloc0(PATH_MAX);
> > +    char *desc_filename = g_malloc0(PATH_MAX);
> > +    char *parent_desc_line = g_malloc0(BUF_SIZE);
> > +    bool zeroed_grain;
> > +    bool compat6;
> > +    int i;
> > +    VMDKCreateOptsData data;
> > +
> > +    if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
> > +        ret = -EINVAL;
> > +        goto exit;
> > +    }
> > +    /* Read out options */
> > +    total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> > +                          BDRV_SECTOR_SIZE);
> > +    adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
> > +    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> > +    hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION);
> > +    compat6 = qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false);
> > +    if (strcmp(hw_version, "undefined") == 0) {
> > +        g_free(hw_version);
> > +        hw_version = g_strdup("4");
> > +    }
> > +    fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
> > +    zeroed_grain = qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false);
> > +
> > +    if (adapter_type) {
> > +        for (i = 0; i < strlen(adapter_type); ++i) {
> > +            adapter_type[i] = qemu_tolower(adapter_type[i]);
> > +        }
> 
> First, you convert to lower cases, and then...
> 
> > +        adapter_type_enum = qapi_enum_parse_full(&BlockdevVmdkAdapterType_lookup,
> > +                                                 adapter_type,
> > +                                                 BLOCKDEV_VMDK_ADAPTER_TYPE_IDE,
> > +                                                 true,
> > +                                                 &local_err);
> 
> ... you parse case-insensitive.  Huh?

I forgot to update this one after adding the qapi_enum_parse_full patch.

> 
> Which spellings did the old code accept?  As far as I can tell, exactly
> "ide", "lsilogic", "buslogic", "legacyESX".  Are you sure we should
> ignore case going forward?

So this comes to the same point as subformat: could QAPI do camelCase as in
"monolithicSparse" and "legacyESX"?

> 
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            ret = -EINVAL;
> > +            goto exit;
> > +        }
> > +    } else {
> > +        adapter_type_enum = BLOCKDEV_VMDK_ADAPTER_TYPE_IDE;
> > +    }
> > +
> > +    if (!fmt) {
> > +        /* Default format to monolithicSparse */
> > +        subformat = BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE;
> > +    } else {
> > +        subformat = qapi_enum_parse_full(&BlockdevVmdkSubformat_lookup,
> > +                                         fmt,
> > +                                         BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE,
> > +                                         true,
> > +                                         &local_err);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            ret = -EINVAL;
> > +            goto exit;
> > +        }
> 
> Likewise: should we ignore case going forward?  The old code appears to
> accept exactly "monolithicFlat", "monolithicSparse",
> "twoGbMaxExtentSparse", "twoGbMaxExtentFlat", "streamOptimized".
> 
> > +    }
> > +    data = (VMDKCreateOptsData){
> > +        .prefix = prefix,
> > +        .postfix = postfix,
> > +        .path = path,
> > +        .opts = opts,
> > +    };
> > +    ret = vmdk_co_do_create(total_size, subformat, adapter_type_enum,
> > +                            backing_file, hw_version, compat6, zeroed_grain,
> > +                            vmdk_co_create_opts_cb, &data, errp);
> > +
> > +exit:
> >      g_free(adapter_type);
> >      g_free(backing_file);
> >      g_free(hw_version);
> > @@ -2156,7 +2301,84 @@ exit:
> >      g_free(ext_filename);
> >      g_free(desc_filename);
> >      g_free(parent_desc_line);
> > -    g_string_free(ext_desc_lines, true);
> > +    return ret;
> > +}
> > +
> > +static BlockBackend *vmdk_co_create_cb(int64_t size, int idx,
> > +                                       bool flat, bool split, bool compress,
> > +                                       bool zeroed_grain, void *opaque,
> > +                                       Error **errp)
> > +{
> > +    int ret;
> > +    BlockDriverState *bs;
> > +    BlockBackend *blk;
> > +    BlockdevCreateOptionsVmdk *opts = opaque;
> > +
> > +    if (idx == 0) {
> > +        bs = bdrv_open_blockdev_ref(opts->file, errp);
> > +    } else {
> > +        int i;
> > +        BlockdevRefList *list = opts->extents;
> > +        for (i = 1; i < idx; i++) {
> > +            if (!list || !list->next) {
> > +                error_setg(errp, "Extent [%d] not specified", i);
> > +                return NULL;
> > +            }
> > +            list = list->next;
> > +        }
> > +        if (!list) {
> > +            error_setg(errp, "Extent [%d] not specified", idx - 1);
> > +            return NULL;
> > +        }
> > +        bs = bdrv_open_blockdev_ref(list->value, errp);
> > +    }
> > +    if (!bs) {
> > +        return NULL;
> > +    }
> > +    blk = blk_new(BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE,
> > +                  BLK_PERM_ALL);
> > +    if (blk_insert_bs(blk, bs, errp)) {
> > +        bdrv_unref(bs);
> > +        return NULL;
> > +    }
> > +    blk_set_allow_write_beyond_eof(blk, true);
> > +    bdrv_unref(bs);
> > +
> > +    ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain, errp);
> > +    if (ret) {
> > +        blk_unref(blk);
> > +        blk = NULL;
> > +    }
> > +    return blk;
> > +}
> > +
> > +static int coroutine_fn vmdk_co_create(BlockdevCreateOptions *create_options,
> > +                                       Error **errp)
> > +{
> > +    int ret;
> > +    BlockdevCreateOptionsVmdk *opts;
> > +
> > +    opts = &create_options->u.vmdk;
> > +
> > +    /* Validate options */
> > +    if (!QEMU_IS_ALIGNED(opts->size, BDRV_SECTOR_SIZE)) {
> > +        error_setg(errp, "Image size must be a multiple of 512 bytes");
> > +        ret = -EINVAL;
> > +        goto out;
> > +    }
> > +
> > +    ret = vmdk_co_do_create(opts->size,
> > +                            opts->subformat,
> > +                            opts->adapter_type,
> > +                            opts->backing_file,
> > +                            opts->hwversion,
> > +                            false,
> > +                            opts->zeroed_grain,
> > +                            vmdk_co_create_cb,
> > +                            opts, errp);
> > +    return ret;
> > +
> > +out:
> >      return ret;
> >  }
> >  
> > @@ -2424,6 +2646,7 @@ static BlockDriver bdrv_vmdk = {
> >      .bdrv_co_pwrite_zeroes        = vmdk_co_pwrite_zeroes,
> >      .bdrv_close                   = vmdk_close,
> >      .bdrv_co_create_opts          = vmdk_co_create_opts,
> > +    .bdrv_co_create               = vmdk_co_create,
> >      .bdrv_co_flush_to_disk        = vmdk_co_flush,
> >      .bdrv_co_block_status         = vmdk_co_block_status,
> >      .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index c50517bff3..df3903b54d 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3855,6 +3855,71 @@
> >              'size':             'size',
> >              '*cluster-size' :   'size' } }
> >  
> > +##
> > +# @BlockdevVmdkSubformat:
> > +#
> > +# Subformat options for VMDK images
> > +#
> > +# @monolithicsparse: Single file image with sparse cluster allocation
> > +# @monolithicflat: Single flat data image and a descriptor file
> > +# @twogbmaxextentsparse: Data is split into 2GB (per virtual LBA) sparse extent
> > +#                        files, in addition to a descriptor file
> > +# @twogbmaxextentflat: Data is split into 2GB (per virtual LBA) flat extent
> > +#                        files, in addition to a descriptor file
> > +# @streamoptimized: Single file image sparse cluster allocation, optimized for
> > +#                   streaming over network.
> > +#
> > +# Since: 2.13
> > +##
> > +{ 'enum': 'BlockdevVmdkSubformat',
> > +  'data': [ 'monolithicsparse', 'monolithicflat', 'twogbmaxextentsparse',
> > +            'twogbmaxextentflat', 'streamoptimized'] }
> 
> alllowercasewithoutspacesisevenlesslegiblethanCamelCase.
> THE RESULTING C IDENTIFIERS ARE ALL CAPS WITHOUT SPACES WHICH IS EVEN WORSE.
> 
> QAPI conventions ask for monolithic-sparse, monolithic-flat,
> two-gb-max-extent-sparse and so forth.  Results in C enum identifiers
> BLOCKDEV_VMDK_SUBFORMAT_MONOLITHIC_SPARSE,
> BLOCKDEV_VMDK_SUBFORMAT_MONOLITHIC_FLAT,
> BLOCKDEV_VMDK_SUBFORMAT_TWO_GB_MAX_EXTENT_SPARSE, ...
> 
> The existing external interface appears to ask for monolithicFlat,
> monolithicSparse, twoGbMaxExtentSparse, ...  What's the best way to map
> between these guys and a QAPI enum?

It would be best if we can stick to monolithicSparse everywhere. Can we?

> 
> > +
> > +##
> > +# @BlockdevVmdkAdapterType:
> > +#
> > +# Adapter type info for VMDK images
> > +#
> > +# Since: 2.13
> > +##
> > +{ 'enum': 'BlockdevVmdkAdapterType',
> > +  'data': [ 'ide', 'buslogic', 'lsilogic', 'legacyesx'] }
> > +
> > +##
> > +# @BlockdevCreateOptionsVmdk:
> > +#
> > +# Driver specific image creation options for VMDK.
> > +#
> > +# @file         Where to store the new image file. This refers to the image
> > +#               file for monolithcSparse and streamOptimized format, or the
> > +#               descriptor file for other formats.
> > +# @size         Size of the virtual disk in bytes
> > +# @extents      Where to store the data extents. Required for monolithcflat,
> > +#               twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For
> > +#               monolithicflat, only one entry is required; for
> > +#               twoGbMaxExtent* formats, the number of entries required is
> > +#               calculated as extent_number = virtual_size / 2GB.
> > +# @subformat    The subformat of the VMDK image. Default: "monolithicsparse".
> > +# @backing-file The path of backing file. Default: no backing file is used.
> > +# @adapter-type The adapter type used to fill in the descriptor. Default: ide.
> > +# @hwversion    Hardware version. The meaningful options are "4" or "6".
> 
> Okay, these are the meaningfull options.  What are the meaningless ones?

I don't know. Historically we've used '3', I have never seen '5'. VMware
articles mention '7' [1]. This is not documented anywhere, so I'm only
listing what has been used by QEMU.

[1]: https://kb.vmware.com/s/article/1026254

> 
> > +#               Defaulted to "4".
> 
> More common phrasings are
> 
>     Default is "4"
>     Defaults to "4"
>     Default: "4"

OK.

> 
> > +# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats.
> > +#               Default: false.
> > +#
> > +# Since: 2.13
> > +##
> > +{ 'struct': 'BlockdevCreateOptionsVmdk',
> > +  'data': { 'file':             'BlockdevRef',
> > +            'size':             'size',
> > +            '*extents':          ['BlockdevRef'],
> > +            '*subformat':       'BlockdevVmdkSubformat',
> > +            '*backing-file':    'str',
> > +            '*adapter-type':    'BlockdevVmdkAdapterType',
> > +            '*hwversion':       'str',
> > +            '*zeroed-grain':    'bool' } }
> > +
> > +
> >  ##
> >  # @SheepdogRedundancyType:
> >  #
> > @@ -4078,7 +4143,7 @@
> >        'throttle':       'BlockdevCreateNotSupported',
> >        'vdi':            'BlockdevCreateOptionsVdi',
> >        'vhdx':           'BlockdevCreateOptionsVhdx',
> > -      'vmdk':           'BlockdevCreateNotSupported',
> > +      'vmdk':           'BlockdevCreateOptionsVmdk',
> >        'vpc':            'BlockdevCreateOptionsVpc',
> >        'vvfat':          'BlockdevCreateNotSupported',
> >        'vxhs':           'BlockdevCreateNotSupported'

Fam
Markus Armbruster May 9, 2018, 6:59 p.m. UTC | #3
Fam Zheng <famz@redhat.com> writes:

> On Wed, 05/09 14:41, Markus Armbruster wrote:
>> Beware, I'm only looking at QAPI-related aspects.
>> 
>> Fam Zheng <famz@redhat.com> writes:
>> 
>> > This makes VMDK support x-blockdev-create. The implementation reuses the
>> > image creation code in vmdk_co_create_opts which now acceptes a callback
>> > pointer to "retrieve" BlockBackend pointers from the caller. This way we
>> > separate the logic between file/extent acquisition and initialization.
>> >
>> > The QAPI command parameters are mostly the same as the old create_opts
>> > except the dropped legacy @compat6 switch, which is redundant with
>> > @hwversion.
>> >
>> > Signed-off-by: Fam Zheng <famz@redhat.com>
>> > ---
>> >  block/vmdk.c         | 481 +++++++++++++++++++++++++++++++++++++--------------
>> >  qapi/block-core.json |  67 ++++++-
>> >  2 files changed, 418 insertions(+), 130 deletions(-)
>> >
>> > diff --git a/block/vmdk.c b/block/vmdk.c
>> > index 083942f806..e16b04e26a 100644
>> > --- a/block/vmdk.c
>> > +++ b/block/vmdk.c
>> > @@ -1905,38 +1905,87 @@ static int filename_decompose(const char *filename, char *path, char *prefix,
>> >      return VMDK_OK;
>> >  }
>> >  
>> > -static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
>> > -                                            Error **errp)
>> > +/* Stringify BlockdevVmdkSubformat enum in the faimiliar camelCase. */
>> > +static const char *vmdk_subformat_str(BlockdevVmdkSubformat subformat)
>> >  {
>> > -    int idx = 0;
>> > -    BlockBackend *new_blk = NULL;
>> > +    switch (subformat) {
>> > +    case BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE:
>> > +        return "monolithicSparse";
>> > +    case BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICFLAT:
>> > +        return "monolithicFlat";
>> > +    case BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT:
>> > +        return "twoGbMaxExtentFlat";
>> > +    case BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTSPARSE:
>> > +        return "twoGbMaxExtentSparse";
>> > +    case BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED:
>> > +        return "streamOptimized";
>> > +    default:
>> > +        abort();
>> > +    }
>> > +}
>> 
>> I'd use an array instead of a switch.
>> 
>> The standard mapping from enum FOO to string is FOO_lookup[], commonly
>> used via qapi_enum_lookup().
>> 
>> vmdk_subformat_str() differs from BlockdevVmdkSubformat_lookup[] only in
>> case: the former is CamelCase ("monolithicSparse"), the latter is all
>> lower case (like "monolithicsparse"), because that's how it's spelled in
>> the QAPI schema.  By the way, QAPI naming conventions ask for hyphens,
>> like "monolithic-sparse".
>> 
>> Why do you need CamelCase?  Is it for an existing external interface?
>> 
>> If yes, should we use CamelCase in the schema?
>> 
>> Should we use hyphens, and have this function map hyphen followed by
>> lower case letter to upper case letter?
>
> "streamOptimized" is the exact style as spelled in the VMDK spec:
>
> https://www.vmware.com/support/developer/vddk/vmdk_50_technote.pdf
>
> so I want to stay as close as possible to it. In other words lower case
> "monolithicsparse" feels a bit better than hyphens "monolithic-sparse" in the
> QAPI interface.

To me it feels like a compromise that makes all parties unhappy: it
deviates from QAPI naming conventions (pain), and differs from VMDK's
spelling (for no gain: we still need extra conversion code).

>> > +
>> > +/*
>> > + * idx == 0: get or create the descriptor file (also the image file if in a
>> > + *           non-split format.
>> > + * idx >= 1: get the n-th extent if in a split subformat
>> > + */
>> > +typedef BlockBackend *(*vmdk_create_extent_fn)(int64_t size,
>> > +                                               int idx,
>> > +                                               bool flat,
>> > +                                               bool split,
>> > +                                               bool compress,
>> > +                                               bool zeroed_grain,
>> > +                                               void *opaque,
>> > +                                               Error **errp);
>> > +
>> > +static void vmdk_desc_add_extent(GString *desc,
>> > +                                 const char *extent_line_fmt,
>> > +                                 int64_t size, const char *filename)
>> > +{
>> > +    char *desc_line = g_malloc0(BUF_SIZE);
>> > +    const char *basename = strrchr(filename, '/');
>> 
>> Blank line between declarations and statements, if you don't mind.
>
> OK!
>
>> 
>> > +    if (!basename) {
>> > +        basename = filename;
>> > +    } else {
>> > +        basename += 1;
>> > +    }
>> 
>> g_path_get_basename()?
>
> Good suggestion, thanks!
>
>> 
>> > +    snprintf(desc_line, BUF_SIZE, extent_line_fmt,
>> > +             DIV_ROUND_UP(size, BDRV_SECTOR_SIZE),
>> > +             basename);
>> > +    g_string_append(desc, desc_line);
>> > +    g_free(desc_line);
>> 
>> g_string_append_printf()?
>
> Yes!
>
>> 
>> > +}
>> > +
>> > +static int coroutine_fn vmdk_co_do_create(int64_t size,
>> > +                                          BlockdevVmdkSubformat subformat,
>> > +                                          BlockdevVmdkAdapterType adapter_type,
>> > +                                          const char *backing_file,
>> > +                                          const char *hw_version,
>> > +                                          bool compat6,
>> > +                                          bool zeroed_grain,
>> > +                                          vmdk_create_extent_fn extent_fn,
>> > +                                          void *opaque,
>> > +                                          Error **errp)
>> > +{
>> > +    int extent_idx;
>> > +    BlockBackend *blk;
>> >      Error *local_err = NULL;
>> >      char *desc = NULL;
>> > -    int64_t total_size = 0, filesize;
>> > -    char *adapter_type = NULL;
>> > -    char *backing_file = NULL;
>> > -    char *hw_version = NULL;
>> > -    char *fmt = NULL;
>> >      int ret = 0;
>> >      bool flat, split, compress;
>> >      GString *ext_desc_lines;
>> > -    char *path = g_malloc0(PATH_MAX);
>> > -    char *prefix = g_malloc0(PATH_MAX);
>> > -    char *postfix = g_malloc0(PATH_MAX);
>> > -    char *desc_line = g_malloc0(BUF_SIZE);
>> > -    char *ext_filename = g_malloc0(PATH_MAX);
>> > -    char *desc_filename = g_malloc0(PATH_MAX);
>> >      const int64_t split_size = 0x80000000;  /* VMDK has constant split size */
>> > -    const char *desc_extent_line;
>> > +    int64_t extent_size;
>> > +    int64_t created_size = 0;
>> > +    const char *extent_line_fmt;
>> >      char *parent_desc_line = g_malloc0(BUF_SIZE);
>> >      uint32_t parent_cid = 0xffffffff;
>> >      uint32_t number_heads = 16;
>> > -    bool zeroed_grain = false;
>> >      uint32_t desc_offset = 0, desc_len;
>> >      const char desc_template[] =
>> >          "# Disk DescriptorFile\n"
>> >          "version=1\n"
>> > -        "CID=%" PRIx32 "\n"
>> > +        "CID=%08" PRIx32 "\n"
>> 
>> How is this change related to the patch's purpose?
>
> It's unrelated. I forgot to remove it after testing: unifying the CID print
> length helps hexdump based compare of monolithicSparse headers.
>
>> 
>> >          "parentCID=%" PRIx32 "\n"
>> >          "createType=\"%s\"\n"
>> >          "%s"
>> > @@ -1955,71 +2004,35 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
>> >  
>> >      ext_desc_lines = g_string_new(NULL);
>> >  
>> > -    if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
>> > -        ret = -EINVAL;
>> > -        goto exit;
>> > -    }
>> >      /* Read out options */
>> > -    total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
>> > -                          BDRV_SECTOR_SIZE);
>> > -    adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
>> > -    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
>> > -    hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION);
>> > -    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false)) {
>> > -        if (strcmp(hw_version, "undefined")) {
>> > +    if (compat6) {
>> > +        if (hw_version) {
>> >              error_setg(errp,
>> >                         "compat6 cannot be enabled with hwversion set");
>> >              ret = -EINVAL;
>> >              goto exit;
>> >          }
>> > -        g_free(hw_version);
>> > -        hw_version = g_strdup("6");
>> > +        hw_version = "6";
>> >      }
>> > -    if (strcmp(hw_version, "undefined") == 0) {
>> > -        g_free(hw_version);
>> > -        hw_version = g_strdup("4");
>> > -    }
>> > -    fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
>> > -    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false)) {
>> > -        zeroed_grain = true;
>> > +    if (!hw_version) {
>> > +        hw_version = "4";
>> >      }
>> >  
>> > -    if (!adapter_type) {
>> > -        adapter_type = g_strdup("ide");
>> > -    } else if (strcmp(adapter_type, "ide") &&
>> > -               strcmp(adapter_type, "buslogic") &&
>> > -               strcmp(adapter_type, "lsilogic") &&
>> > -               strcmp(adapter_type, "legacyESX")) {
>> > -        error_setg(errp, "Unknown adapter type: '%s'", adapter_type);
>> > -        ret = -EINVAL;
>> > -        goto exit;
>> > -    }
>> > -    if (strcmp(adapter_type, "ide") != 0) {
>> > +    if (adapter_type != BLOCKDEV_VMDK_ADAPTER_TYPE_IDE) {
>> >          /* that's the number of heads with which vmware operates when
>> >             creating, exporting, etc. vmdk files with a non-ide adapter type */
>> >          number_heads = 255;
>> >      }
>> > -    if (!fmt) {
>> > -        /* Default format to monolithicSparse */
>> > -        fmt = g_strdup("monolithicSparse");
>> > -    } else if (strcmp(fmt, "monolithicFlat") &&
>> > -               strcmp(fmt, "monolithicSparse") &&
>> > -               strcmp(fmt, "twoGbMaxExtentSparse") &&
>> > -               strcmp(fmt, "twoGbMaxExtentFlat") &&
>> > -               strcmp(fmt, "streamOptimized")) {
>> > -        error_setg(errp, "Unknown subformat: '%s'", fmt);
>> > -        ret = -EINVAL;
>> > -        goto exit;
>> > -    }
>> > -    split = !(strcmp(fmt, "twoGbMaxExtentFlat") &&
>> > -              strcmp(fmt, "twoGbMaxExtentSparse"));
>> > -    flat = !(strcmp(fmt, "monolithicFlat") &&
>> > -             strcmp(fmt, "twoGbMaxExtentFlat"));
>> > -    compress = !strcmp(fmt, "streamOptimized");
>> > +    split = (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT) ||
>> > +            (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTSPARSE);
>> > +    flat = (subformat == BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICFLAT) ||
>> > +           (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT);
>> > +    compress = subformat == BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED;
>> > +
>> >      if (flat) {
>> > -        desc_extent_line = "RW %" PRId64 " FLAT \"%s\" 0\n";
>> > +        extent_line_fmt = "RW %" PRId64 " FLAT \"%s\" 0\n";
>> >      } else {
>> > -        desc_extent_line = "RW %" PRId64 " SPARSE \"%s\"\n";
>> > +        extent_line_fmt = "RW %" PRId64 " SPARSE \"%s\"\n";
>> >      }
>> >      if (flat && backing_file) {
>> >          error_setg(errp, "Flat image can't have backing file");
>> > @@ -2031,10 +2044,34 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
>> >          ret = -ENOTSUP;
>> >          goto exit;
>> >      }
>> > +
>> > +    /* Create extents */
>> > +    if (split) {
>> > +        extent_size = split_size;
>> > +    } else {
>> > +        extent_size = size;
>> > +    }
>> > +    if (!split && !flat) {
>> > +        created_size = extent_size;
>> > +    } else {
>> > +        created_size = 0;
>> > +    }
>> > +    /* Get the descriptor file BDS */
>> > +    blk = extent_fn(created_size, 0, flat, split, compress, zeroed_grain,
>> > +                    opaque, errp);
>> > +    if (!blk) {
>> > +        ret = -EIO;
>> > +        goto exit;
>> > +    }
>> > +    if (!split && !flat) {
>> > +        vmdk_desc_add_extent(ext_desc_lines, extent_line_fmt, created_size,
>> > +                             blk_bs(blk)->filename);
>> > +    }
>> > +
>> >      if (backing_file) {
>> > -        BlockBackend *blk;
>> > +        BlockBackend *backing;
>> >          char *full_backing = g_new0(char, PATH_MAX);
>> > -        bdrv_get_full_backing_filename_from_filename(filename, backing_file,
>> > +        bdrv_get_full_backing_filename_from_filename(blk_bs(blk)->filename, backing_file,
>> >                                                       full_backing, PATH_MAX,
>> >                                                       &local_err);
>> >          if (local_err) {
>> > @@ -2044,93 +2081,65 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
>> >              goto exit;
>> >          }
>> >  
>> > -        blk = blk_new_open(full_backing, NULL, NULL,
>> > -                           BDRV_O_NO_BACKING, errp);
>> > +        backing = blk_new_open(full_backing, NULL, NULL,
>> > +                               BDRV_O_NO_BACKING, errp);
>> >          g_free(full_backing);
>> > -        if (blk == NULL) {
>> > +        if (backing == NULL) {
>> >              ret = -EIO;
>> >              goto exit;
>> >          }
>> > -        if (strcmp(blk_bs(blk)->drv->format_name, "vmdk")) {
>> > -            blk_unref(blk);
>> > +        if (strcmp(blk_bs(backing)->drv->format_name, "vmdk")) {
>> > +            error_setg(errp, "Invalid backing file format: %s. Must be vmdk",
>> > +                       blk_bs(backing)->drv->format_name);
>> > +            blk_unref(backing);
>> >              ret = -EINVAL;
>> >              goto exit;
>> >          }
>> > -        ret = vmdk_read_cid(blk_bs(blk), 0, &parent_cid);
>> > -        blk_unref(blk);
>> > +        ret = vmdk_read_cid(blk_bs(backing), 0, &parent_cid);
>> > +        blk_unref(backing);
>> >          if (ret) {
>> > +            error_setg(errp, "Failed to read parent CID");
>> >              goto exit;
>> >          }
>> >          snprintf(parent_desc_line, BUF_SIZE,
>> >                  "parentFileNameHint=\"%s\"", backing_file);
>> >      }
>> > -
>> > -    /* Create extents */
>> > -    filesize = total_size;
>> > -    while (filesize > 0) {
>> > -        int64_t size = filesize;
>> > -
>> > -        if (split && size > split_size) {
>> > -            size = split_size;
>> > -        }
>> > -        if (split) {
>> > -            snprintf(desc_filename, PATH_MAX, "%s-%c%03d%s",
>> > -                    prefix, flat ? 'f' : 's', ++idx, postfix);
>> > -        } else if (flat) {
>> > -            snprintf(desc_filename, PATH_MAX, "%s-flat%s", prefix, postfix);
>> > -        } else {
>> > -            snprintf(desc_filename, PATH_MAX, "%s%s", prefix, postfix);
>> > -        }
>> > -        snprintf(ext_filename, PATH_MAX, "%s%s", path, desc_filename);
>> > -
>> > -        if (vmdk_create_extent(ext_filename, size,
>> > -                               flat, compress, zeroed_grain, NULL, opts, errp)) {
>> > +    extent_idx = 1;
>> > +    while (created_size < size) {
>> > +        BlockBackend *extent_blk;
>> > +        int64_t cur_size = MIN(size - created_size, extent_size);
>> > +        extent_blk = extent_fn(cur_size, extent_idx, flat, split, compress,
>> > +                               zeroed_grain, opaque, errp);
>> > +        if (!extent_blk) {
>> >              ret = -EINVAL;
>> >              goto exit;
>> >          }
>> > -        filesize -= size;
>> > -
>> > -        /* Format description line */
>> > -        snprintf(desc_line, BUF_SIZE,
>> > -                    desc_extent_line, size / BDRV_SECTOR_SIZE, desc_filename);
>> > -        g_string_append(ext_desc_lines, desc_line);
>> > +        vmdk_desc_add_extent(ext_desc_lines, extent_line_fmt, cur_size,
>> > +                             blk_bs(extent_blk)->filename);
>> > +        created_size += cur_size;
>> > +        extent_idx++;
>> > +        blk_unref(extent_blk);
>> >      }
>> >      /* generate descriptor file */
>> >      desc = g_strdup_printf(desc_template,
>> >                             g_random_int(),
>> >                             parent_cid,
>> > -                           fmt,
>> > +                           vmdk_subformat_str(subformat),
>> >                             parent_desc_line,
>> >                             ext_desc_lines->str,
>> >                             hw_version,
>> > -                           total_size /
>> > +                           size /
>> >                                 (int64_t)(63 * number_heads * BDRV_SECTOR_SIZE),
>> >                             number_heads,
>> > -                           adapter_type);
>> > +                           qapi_enum_lookup(&BlockdevVmdkAdapterType_lookup,
>> > +                                            adapter_type));
>> >      desc_len = strlen(desc);
>> >      /* the descriptor offset = 0x200 */
>> >      if (!split && !flat) {
>> >          desc_offset = 0x200;
>> > -    } else {
>> > -        ret = bdrv_create_file(filename, opts, &local_err);
>> > -        if (ret < 0) {
>> > -            error_propagate(errp, local_err);
>> > -            goto exit;
>> > -        }
>> >      }
>> >  
>> > -    new_blk = blk_new_open(filename, NULL, NULL,
>> > -                           BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
>> > -                           &local_err);
>> > -    if (new_blk == NULL) {
>> > -        error_propagate(errp, local_err);
>> > -        ret = -EIO;
>> > -        goto exit;
>> > -    }
>> > -
>> > -    blk_set_allow_write_beyond_eof(new_blk, true);
>> > -
>> > -    ret = blk_pwrite(new_blk, desc_offset, desc, desc_len, 0);
>> > +    ret = blk_pwrite(blk, desc_offset, desc, desc_len, 0);
>> >      if (ret < 0) {
>> >          error_setg_errno(errp, -ret, "Could not write description");
>> >          goto exit;
>> > @@ -2138,12 +2147,148 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
>> >      /* bdrv_pwrite write padding zeros to align to sector, we don't need that
>> >       * for description file */
>> >      if (desc_offset == 0) {
>> > -        ret = blk_truncate(new_blk, desc_len, PREALLOC_MODE_OFF, errp);
>> > +        ret = blk_truncate(blk, desc_len, PREALLOC_MODE_OFF, errp);
>> >      }
>> >  exit:
>> > -    if (new_blk) {
>> > -        blk_unref(new_blk);
>> > +    if (blk) {
>> > +        blk_unref(blk);
>> >      }
>> > +    g_free(desc);
>> > +    g_free(parent_desc_line);
>> > +    g_string_free(ext_desc_lines, true);
>> > +    return ret;
>> > +}
>> > +
>> > +typedef struct {
>> > +    char *path;
>> > +    char *prefix;
>> > +    char *postfix;
>> > +    QemuOpts *opts;
>> > +} VMDKCreateOptsData;
>> > +
>> > +static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
>> > +                                            bool flat, bool split, bool compress,
>> > +                                            bool zeroed_grain, void *opaque,
>> > +                                            Error **errp)
>> > +{
>> > +    BlockBackend *blk = NULL;
>> > +    BlockDriverState *bs = NULL;
>> > +    VMDKCreateOptsData *data = opaque;
>> > +    char *ext_filename = NULL;
>> > +    char *rel_filename = NULL;
>> > +
>> > +    if (idx == 0) {
>> > +        rel_filename = g_strdup_printf("%s%s", data->prefix, data->postfix);
>> > +    } else if (split) {
>> > +        rel_filename = g_strdup_printf("%s-%c%03d%s",
>> > +                                       data->prefix,
>> > +                                       flat ? 'f' : 's', idx, data->postfix);
>> > +    } else {
>> > +        assert(idx == 1);
>> > +        rel_filename = g_strdup_printf("%s-flat%s", data->prefix, data->postfix);
>> > +    }
>> > +
>> > +    ext_filename = g_strdup_printf("%s%s", data->path, rel_filename);
>> > +    g_free(rel_filename);
>> > +
>> > +    if (vmdk_create_extent(ext_filename, size,
>> > +                           flat, compress, zeroed_grain, &blk, data->opts,
>> > +                           errp)) {
>> > +        goto exit;
>> > +    }
>> > +    bdrv_unref(bs);
>> > +exit:
>> > +    g_free(ext_filename);
>> > +    return blk;
>> > +}
>> > +
>> > +static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
>> > +                                            Error **errp)
>> > +{
>> > +    Error *local_err = NULL;
>> > +    char *desc = NULL;
>> > +    int64_t total_size = 0;
>> > +    char *adapter_type = NULL;
>> > +    BlockdevVmdkAdapterType adapter_type_enum;
>> > +    char *backing_file = NULL;
>> > +    char *hw_version = NULL;
>> > +    char *fmt = NULL;
>> > +    BlockdevVmdkSubformat subformat;
>> > +    int ret = 0;
>> > +    char *path = g_malloc0(PATH_MAX);
>> > +    char *prefix = g_malloc0(PATH_MAX);
>> > +    char *postfix = g_malloc0(PATH_MAX);
>> > +    char *desc_line = g_malloc0(BUF_SIZE);
>> > +    char *ext_filename = g_malloc0(PATH_MAX);
>> > +    char *desc_filename = g_malloc0(PATH_MAX);
>> > +    char *parent_desc_line = g_malloc0(BUF_SIZE);
>> > +    bool zeroed_grain;
>> > +    bool compat6;
>> > +    int i;
>> > +    VMDKCreateOptsData data;
>> > +
>> > +    if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
>> > +        ret = -EINVAL;
>> > +        goto exit;
>> > +    }
>> > +    /* Read out options */
>> > +    total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
>> > +                          BDRV_SECTOR_SIZE);
>> > +    adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
>> > +    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
>> > +    hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION);
>> > +    compat6 = qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false);
>> > +    if (strcmp(hw_version, "undefined") == 0) {
>> > +        g_free(hw_version);
>> > +        hw_version = g_strdup("4");
>> > +    }
>> > +    fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
>> > +    zeroed_grain = qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false);
>> > +
>> > +    if (adapter_type) {
>> > +        for (i = 0; i < strlen(adapter_type); ++i) {
>> > +            adapter_type[i] = qemu_tolower(adapter_type[i]);
>> > +        }
>> 
>> First, you convert to lower cases, and then...
>> 
>> > +        adapter_type_enum = qapi_enum_parse_full(&BlockdevVmdkAdapterType_lookup,
>> > +                                                 adapter_type,
>> > +                                                 BLOCKDEV_VMDK_ADAPTER_TYPE_IDE,
>> > +                                                 true,
>> > +                                                 &local_err);
>> 
>> ... you parse case-insensitive.  Huh?
>
> I forgot to update this one after adding the qapi_enum_parse_full patch.
>
>> 
>> Which spellings did the old code accept?  As far as I can tell, exactly
>> "ide", "lsilogic", "buslogic", "legacyESX".  Are you sure we should
>> ignore case going forward?
>
> So this comes to the same point as subformat: could QAPI do camelCase as in
> "monolithicSparse" and "legacyESX"?

Yes, but you might have to add the type to the name-case-whitelist in
qapi-schema.json.

Additions to name-case-whitelist need a really good reason.  Making code
simpler could be one.

>> > +        if (local_err) {
>> > +            error_propagate(errp, local_err);
>> > +            ret = -EINVAL;
>> > +            goto exit;
>> > +        }
>> > +    } else {
>> > +        adapter_type_enum = BLOCKDEV_VMDK_ADAPTER_TYPE_IDE;
>> > +    }
>> > +
>> > +    if (!fmt) {
>> > +        /* Default format to monolithicSparse */
>> > +        subformat = BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE;
>> > +    } else {
>> > +        subformat = qapi_enum_parse_full(&BlockdevVmdkSubformat_lookup,
>> > +                                         fmt,
>> > +                                         BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE,
>> > +                                         true,
>> > +                                         &local_err);
>> > +        if (local_err) {
>> > +            error_propagate(errp, local_err);
>> > +            ret = -EINVAL;
>> > +            goto exit;
>> > +        }
>> 
>> Likewise: should we ignore case going forward?  The old code appears to
>> accept exactly "monolithicFlat", "monolithicSparse",
>> "twoGbMaxExtentSparse", "twoGbMaxExtentFlat", "streamOptimized".
>> 
>> > +    }
>> > +    data = (VMDKCreateOptsData){
>> > +        .prefix = prefix,
>> > +        .postfix = postfix,
>> > +        .path = path,
>> > +        .opts = opts,
>> > +    };
>> > +    ret = vmdk_co_do_create(total_size, subformat, adapter_type_enum,
>> > +                            backing_file, hw_version, compat6, zeroed_grain,
>> > +                            vmdk_co_create_opts_cb, &data, errp);
>> > +
>> > +exit:
>> >      g_free(adapter_type);
>> >      g_free(backing_file);
>> >      g_free(hw_version);
>> > @@ -2156,7 +2301,84 @@ exit:
>> >      g_free(ext_filename);
>> >      g_free(desc_filename);
>> >      g_free(parent_desc_line);
>> > -    g_string_free(ext_desc_lines, true);
>> > +    return ret;
>> > +}
>> > +
>> > +static BlockBackend *vmdk_co_create_cb(int64_t size, int idx,
>> > +                                       bool flat, bool split, bool compress,
>> > +                                       bool zeroed_grain, void *opaque,
>> > +                                       Error **errp)
>> > +{
>> > +    int ret;
>> > +    BlockDriverState *bs;
>> > +    BlockBackend *blk;
>> > +    BlockdevCreateOptionsVmdk *opts = opaque;
>> > +
>> > +    if (idx == 0) {
>> > +        bs = bdrv_open_blockdev_ref(opts->file, errp);
>> > +    } else {
>> > +        int i;
>> > +        BlockdevRefList *list = opts->extents;
>> > +        for (i = 1; i < idx; i++) {
>> > +            if (!list || !list->next) {
>> > +                error_setg(errp, "Extent [%d] not specified", i);
>> > +                return NULL;
>> > +            }
>> > +            list = list->next;
>> > +        }
>> > +        if (!list) {
>> > +            error_setg(errp, "Extent [%d] not specified", idx - 1);
>> > +            return NULL;
>> > +        }
>> > +        bs = bdrv_open_blockdev_ref(list->value, errp);
>> > +    }
>> > +    if (!bs) {
>> > +        return NULL;
>> > +    }
>> > +    blk = blk_new(BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE,
>> > +                  BLK_PERM_ALL);
>> > +    if (blk_insert_bs(blk, bs, errp)) {
>> > +        bdrv_unref(bs);
>> > +        return NULL;
>> > +    }
>> > +    blk_set_allow_write_beyond_eof(blk, true);
>> > +    bdrv_unref(bs);
>> > +
>> > +    ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain, errp);
>> > +    if (ret) {
>> > +        blk_unref(blk);
>> > +        blk = NULL;
>> > +    }
>> > +    return blk;
>> > +}
>> > +
>> > +static int coroutine_fn vmdk_co_create(BlockdevCreateOptions *create_options,
>> > +                                       Error **errp)
>> > +{
>> > +    int ret;
>> > +    BlockdevCreateOptionsVmdk *opts;
>> > +
>> > +    opts = &create_options->u.vmdk;
>> > +
>> > +    /* Validate options */
>> > +    if (!QEMU_IS_ALIGNED(opts->size, BDRV_SECTOR_SIZE)) {
>> > +        error_setg(errp, "Image size must be a multiple of 512 bytes");
>> > +        ret = -EINVAL;
>> > +        goto out;
>> > +    }
>> > +
>> > +    ret = vmdk_co_do_create(opts->size,
>> > +                            opts->subformat,
>> > +                            opts->adapter_type,
>> > +                            opts->backing_file,
>> > +                            opts->hwversion,
>> > +                            false,
>> > +                            opts->zeroed_grain,
>> > +                            vmdk_co_create_cb,
>> > +                            opts, errp);
>> > +    return ret;
>> > +
>> > +out:
>> >      return ret;
>> >  }
>> >  
>> > @@ -2424,6 +2646,7 @@ static BlockDriver bdrv_vmdk = {
>> >      .bdrv_co_pwrite_zeroes        = vmdk_co_pwrite_zeroes,
>> >      .bdrv_close                   = vmdk_close,
>> >      .bdrv_co_create_opts          = vmdk_co_create_opts,
>> > +    .bdrv_co_create               = vmdk_co_create,
>> >      .bdrv_co_flush_to_disk        = vmdk_co_flush,
>> >      .bdrv_co_block_status         = vmdk_co_block_status,
>> >      .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index c50517bff3..df3903b54d 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -3855,6 +3855,71 @@
>> >              'size':             'size',
>> >              '*cluster-size' :   'size' } }
>> >  
>> > +##
>> > +# @BlockdevVmdkSubformat:
>> > +#
>> > +# Subformat options for VMDK images
>> > +#
>> > +# @monolithicsparse: Single file image with sparse cluster allocation
>> > +# @monolithicflat: Single flat data image and a descriptor file
>> > +# @twogbmaxextentsparse: Data is split into 2GB (per virtual LBA) sparse extent
>> > +#                        files, in addition to a descriptor file
>> > +# @twogbmaxextentflat: Data is split into 2GB (per virtual LBA) flat extent
>> > +#                        files, in addition to a descriptor file
>> > +# @streamoptimized: Single file image sparse cluster allocation, optimized for
>> > +#                   streaming over network.
>> > +#
>> > +# Since: 2.13
>> > +##
>> > +{ 'enum': 'BlockdevVmdkSubformat',
>> > +  'data': [ 'monolithicsparse', 'monolithicflat', 'twogbmaxextentsparse',
>> > +            'twogbmaxextentflat', 'streamoptimized'] }
>> 
>> alllowercasewithoutspacesisevenlesslegiblethanCamelCase.
>> THERESULTINGCIDENTIFIERSAREALLCAPSWITHOUTSPACESWHICHISEVENWORSE.
>> 
>> QAPI conventions ask for monolithic-sparse, monolithic-flat,
>> two-gb-max-extent-sparse and so forth.  Results in C enum identifiers
>> BLOCKDEV_VMDK_SUBFORMAT_MONOLITHIC_SPARSE,
>> BLOCKDEV_VMDK_SUBFORMAT_MONOLITHIC_FLAT,
>> BLOCKDEV_VMDK_SUBFORMAT_TWO_GB_MAX_EXTENT_SPARSE, ...
>> 
>> The existing external interface appears to ask for monolithicFlat,
>> monolithicSparse, twoGbMaxExtentSparse, ...  What's the best way to map
>> between these guys and a QAPI enum?
>
> It would be best if we can stick to monolithicSparse everywhere. Can we?

Please try.  The generated C identifiers will be ugly, but a bit of
ugliness is probably less bad than conversion code.

>> > +
>> > +##
>> > +# @BlockdevVmdkAdapterType:
>> > +#
>> > +# Adapter type info for VMDK images
>> > +#
>> > +# Since: 2.13
>> > +##
>> > +{ 'enum': 'BlockdevVmdkAdapterType',
>> > +  'data': [ 'ide', 'buslogic', 'lsilogic', 'legacyesx'] }
>> > +
>> > +##
>> > +# @BlockdevCreateOptionsVmdk:
>> > +#
>> > +# Driver specific image creation options for VMDK.
>> > +#
>> > +# @file         Where to store the new image file. This refers to the image
>> > +#               file for monolithcSparse and streamOptimized format, or the
>> > +#               descriptor file for other formats.
>> > +# @size         Size of the virtual disk in bytes
>> > +# @extents      Where to store the data extents. Required for monolithcflat,
>> > +#               twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For
>> > +#               monolithicflat, only one entry is required; for
>> > +#               twoGbMaxExtent* formats, the number of entries required is
>> > +#               calculated as extent_number = virtual_size / 2GB.
>> > +# @subformat    The subformat of the VMDK image. Default: "monolithicsparse".
>> > +# @backing-file The path of backing file. Default: no backing file is used.
>> > +# @adapter-type The adapter type used to fill in the descriptor. Default: ide.
>> > +# @hwversion    Hardware version. The meaningful options are "4" or "6".
>> 
>> Okay, these are the meaningfull options.  What are the meaningless ones?
>
> I don't know. Historically we've used '3', I have never seen '5'. VMware
> articles mention '7' [1]. This is not documented anywhere, so I'm only
> listing what has been used by QEMU.
>
> [1]: https://kb.vmware.com/s/article/1026254

What about

    # @hwversion    Hardware version.  Recognized values are "4" and "6".

>> 
>> > +#               Defaulted to "4".
>> 
>> More common phrasings are
>> 
>>     Default is "4"
>>     Defaults to "4"
>>     Default: "4"
>
> OK.
>
>> 
>> > +# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats.
>> > +#               Default: false.
>> > +#
>> > +# Since: 2.13
>> > +##
>> > +{ 'struct': 'BlockdevCreateOptionsVmdk',
>> > +  'data': { 'file':             'BlockdevRef',
>> > +            'size':             'size',
>> > +            '*extents':          ['BlockdevRef'],
>> > +            '*subformat':       'BlockdevVmdkSubformat',
>> > +            '*backing-file':    'str',
>> > +            '*adapter-type':    'BlockdevVmdkAdapterType',
>> > +            '*hwversion':       'str',
>> > +            '*zeroed-grain':    'bool' } }
>> > +
>> > +
>> >  ##
>> >  # @SheepdogRedundancyType:
>> >  #
>> > @@ -4078,7 +4143,7 @@
>> >        'throttle':       'BlockdevCreateNotSupported',
>> >        'vdi':            'BlockdevCreateOptionsVdi',
>> >        'vhdx':           'BlockdevCreateOptionsVhdx',
>> > -      'vmdk':           'BlockdevCreateNotSupported',
>> > +      'vmdk':           'BlockdevCreateOptionsVmdk',
>> >        'vpc':            'BlockdevCreateOptionsVpc',
>> >        'vvfat':          'BlockdevCreateNotSupported',
>> >        'vxhs':           'BlockdevCreateNotSupported'
>
> Fam

Patch
diff mbox series

diff --git a/block/vmdk.c b/block/vmdk.c
index 083942f806..e16b04e26a 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1905,38 +1905,87 @@  static int filename_decompose(const char *filename, char *path, char *prefix,
     return VMDK_OK;
 }
 
-static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
-                                            Error **errp)
+/* Stringify BlockdevVmdkSubformat enum in the faimiliar camelCase. */
+static const char *vmdk_subformat_str(BlockdevVmdkSubformat subformat)
 {
-    int idx = 0;
-    BlockBackend *new_blk = NULL;
+    switch (subformat) {
+    case BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE:
+        return "monolithicSparse";
+    case BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICFLAT:
+        return "monolithicFlat";
+    case BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT:
+        return "twoGbMaxExtentFlat";
+    case BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTSPARSE:
+        return "twoGbMaxExtentSparse";
+    case BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED:
+        return "streamOptimized";
+    default:
+        abort();
+    }
+}
+
+/*
+ * idx == 0: get or create the descriptor file (also the image file if in a
+ *           non-split format.
+ * idx >= 1: get the n-th extent if in a split subformat
+ */
+typedef BlockBackend *(*vmdk_create_extent_fn)(int64_t size,
+                                               int idx,
+                                               bool flat,
+                                               bool split,
+                                               bool compress,
+                                               bool zeroed_grain,
+                                               void *opaque,
+                                               Error **errp);
+
+static void vmdk_desc_add_extent(GString *desc,
+                                 const char *extent_line_fmt,
+                                 int64_t size, const char *filename)
+{
+    char *desc_line = g_malloc0(BUF_SIZE);
+    const char *basename = strrchr(filename, '/');
+    if (!basename) {
+        basename = filename;
+    } else {
+        basename += 1;
+    }
+    snprintf(desc_line, BUF_SIZE, extent_line_fmt,
+             DIV_ROUND_UP(size, BDRV_SECTOR_SIZE),
+             basename);
+    g_string_append(desc, desc_line);
+    g_free(desc_line);
+}
+
+static int coroutine_fn vmdk_co_do_create(int64_t size,
+                                          BlockdevVmdkSubformat subformat,
+                                          BlockdevVmdkAdapterType adapter_type,
+                                          const char *backing_file,
+                                          const char *hw_version,
+                                          bool compat6,
+                                          bool zeroed_grain,
+                                          vmdk_create_extent_fn extent_fn,
+                                          void *opaque,
+                                          Error **errp)
+{
+    int extent_idx;
+    BlockBackend *blk;
     Error *local_err = NULL;
     char *desc = NULL;
-    int64_t total_size = 0, filesize;
-    char *adapter_type = NULL;
-    char *backing_file = NULL;
-    char *hw_version = NULL;
-    char *fmt = NULL;
     int ret = 0;
     bool flat, split, compress;
     GString *ext_desc_lines;
-    char *path = g_malloc0(PATH_MAX);
-    char *prefix = g_malloc0(PATH_MAX);
-    char *postfix = g_malloc0(PATH_MAX);
-    char *desc_line = g_malloc0(BUF_SIZE);
-    char *ext_filename = g_malloc0(PATH_MAX);
-    char *desc_filename = g_malloc0(PATH_MAX);
     const int64_t split_size = 0x80000000;  /* VMDK has constant split size */
-    const char *desc_extent_line;
+    int64_t extent_size;
+    int64_t created_size = 0;
+    const char *extent_line_fmt;
     char *parent_desc_line = g_malloc0(BUF_SIZE);
     uint32_t parent_cid = 0xffffffff;
     uint32_t number_heads = 16;
-    bool zeroed_grain = false;
     uint32_t desc_offset = 0, desc_len;
     const char desc_template[] =
         "# Disk DescriptorFile\n"
         "version=1\n"
-        "CID=%" PRIx32 "\n"
+        "CID=%08" PRIx32 "\n"
         "parentCID=%" PRIx32 "\n"
         "createType=\"%s\"\n"
         "%s"
@@ -1955,71 +2004,35 @@  static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
 
     ext_desc_lines = g_string_new(NULL);
 
-    if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
-        ret = -EINVAL;
-        goto exit;
-    }
     /* Read out options */
-    total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
-                          BDRV_SECTOR_SIZE);
-    adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
-    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
-    hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION);
-    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false)) {
-        if (strcmp(hw_version, "undefined")) {
+    if (compat6) {
+        if (hw_version) {
             error_setg(errp,
                        "compat6 cannot be enabled with hwversion set");
             ret = -EINVAL;
             goto exit;
         }
-        g_free(hw_version);
-        hw_version = g_strdup("6");
+        hw_version = "6";
     }
-    if (strcmp(hw_version, "undefined") == 0) {
-        g_free(hw_version);
-        hw_version = g_strdup("4");
-    }
-    fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
-    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false)) {
-        zeroed_grain = true;
+    if (!hw_version) {
+        hw_version = "4";
     }
 
-    if (!adapter_type) {
-        adapter_type = g_strdup("ide");
-    } else if (strcmp(adapter_type, "ide") &&
-               strcmp(adapter_type, "buslogic") &&
-               strcmp(adapter_type, "lsilogic") &&
-               strcmp(adapter_type, "legacyESX")) {
-        error_setg(errp, "Unknown adapter type: '%s'", adapter_type);
-        ret = -EINVAL;
-        goto exit;
-    }
-    if (strcmp(adapter_type, "ide") != 0) {
+    if (adapter_type != BLOCKDEV_VMDK_ADAPTER_TYPE_IDE) {
         /* that's the number of heads with which vmware operates when
            creating, exporting, etc. vmdk files with a non-ide adapter type */
         number_heads = 255;
     }
-    if (!fmt) {
-        /* Default format to monolithicSparse */
-        fmt = g_strdup("monolithicSparse");
-    } else if (strcmp(fmt, "monolithicFlat") &&
-               strcmp(fmt, "monolithicSparse") &&
-               strcmp(fmt, "twoGbMaxExtentSparse") &&
-               strcmp(fmt, "twoGbMaxExtentFlat") &&
-               strcmp(fmt, "streamOptimized")) {
-        error_setg(errp, "Unknown subformat: '%s'", fmt);
-        ret = -EINVAL;
-        goto exit;
-    }
-    split = !(strcmp(fmt, "twoGbMaxExtentFlat") &&
-              strcmp(fmt, "twoGbMaxExtentSparse"));
-    flat = !(strcmp(fmt, "monolithicFlat") &&
-             strcmp(fmt, "twoGbMaxExtentFlat"));
-    compress = !strcmp(fmt, "streamOptimized");
+    split = (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT) ||
+            (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTSPARSE);
+    flat = (subformat == BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICFLAT) ||
+           (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT);
+    compress = subformat == BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED;
+
     if (flat) {
-        desc_extent_line = "RW %" PRId64 " FLAT \"%s\" 0\n";
+        extent_line_fmt = "RW %" PRId64 " FLAT \"%s\" 0\n";
     } else {
-        desc_extent_line = "RW %" PRId64 " SPARSE \"%s\"\n";
+        extent_line_fmt = "RW %" PRId64 " SPARSE \"%s\"\n";
     }
     if (flat && backing_file) {
         error_setg(errp, "Flat image can't have backing file");
@@ -2031,10 +2044,34 @@  static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
         ret = -ENOTSUP;
         goto exit;
     }
+
+    /* Create extents */
+    if (split) {
+        extent_size = split_size;
+    } else {
+        extent_size = size;
+    }
+    if (!split && !flat) {
+        created_size = extent_size;
+    } else {
+        created_size = 0;
+    }
+    /* Get the descriptor file BDS */
+    blk = extent_fn(created_size, 0, flat, split, compress, zeroed_grain,
+                    opaque, errp);
+    if (!blk) {
+        ret = -EIO;
+        goto exit;
+    }
+    if (!split && !flat) {
+        vmdk_desc_add_extent(ext_desc_lines, extent_line_fmt, created_size,
+                             blk_bs(blk)->filename);
+    }
+
     if (backing_file) {
-        BlockBackend *blk;
+        BlockBackend *backing;
         char *full_backing = g_new0(char, PATH_MAX);
-        bdrv_get_full_backing_filename_from_filename(filename, backing_file,
+        bdrv_get_full_backing_filename_from_filename(blk_bs(blk)->filename, backing_file,
                                                      full_backing, PATH_MAX,
                                                      &local_err);
         if (local_err) {
@@ -2044,93 +2081,65 @@  static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
             goto exit;
         }
 
-        blk = blk_new_open(full_backing, NULL, NULL,
-                           BDRV_O_NO_BACKING, errp);
+        backing = blk_new_open(full_backing, NULL, NULL,
+                               BDRV_O_NO_BACKING, errp);
         g_free(full_backing);
-        if (blk == NULL) {
+        if (backing == NULL) {
             ret = -EIO;
             goto exit;
         }
-        if (strcmp(blk_bs(blk)->drv->format_name, "vmdk")) {
-            blk_unref(blk);
+        if (strcmp(blk_bs(backing)->drv->format_name, "vmdk")) {
+            error_setg(errp, "Invalid backing file format: %s. Must be vmdk",
+                       blk_bs(backing)->drv->format_name);
+            blk_unref(backing);
             ret = -EINVAL;
             goto exit;
         }
-        ret = vmdk_read_cid(blk_bs(blk), 0, &parent_cid);
-        blk_unref(blk);
+        ret = vmdk_read_cid(blk_bs(backing), 0, &parent_cid);
+        blk_unref(backing);
         if (ret) {
+            error_setg(errp, "Failed to read parent CID");
             goto exit;
         }
         snprintf(parent_desc_line, BUF_SIZE,
                 "parentFileNameHint=\"%s\"", backing_file);
     }
-
-    /* Create extents */
-    filesize = total_size;
-    while (filesize > 0) {
-        int64_t size = filesize;
-
-        if (split && size > split_size) {
-            size = split_size;
-        }
-        if (split) {
-            snprintf(desc_filename, PATH_MAX, "%s-%c%03d%s",
-                    prefix, flat ? 'f' : 's', ++idx, postfix);
-        } else if (flat) {
-            snprintf(desc_filename, PATH_MAX, "%s-flat%s", prefix, postfix);
-        } else {
-            snprintf(desc_filename, PATH_MAX, "%s%s", prefix, postfix);
-        }
-        snprintf(ext_filename, PATH_MAX, "%s%s", path, desc_filename);
-
-        if (vmdk_create_extent(ext_filename, size,
-                               flat, compress, zeroed_grain, NULL, opts, errp)) {
+    extent_idx = 1;
+    while (created_size < size) {
+        BlockBackend *extent_blk;
+        int64_t cur_size = MIN(size - created_size, extent_size);
+        extent_blk = extent_fn(cur_size, extent_idx, flat, split, compress,
+                               zeroed_grain, opaque, errp);
+        if (!extent_blk) {
             ret = -EINVAL;
             goto exit;
         }
-        filesize -= size;
-
-        /* Format description line */
-        snprintf(desc_line, BUF_SIZE,
-                    desc_extent_line, size / BDRV_SECTOR_SIZE, desc_filename);
-        g_string_append(ext_desc_lines, desc_line);
+        vmdk_desc_add_extent(ext_desc_lines, extent_line_fmt, cur_size,
+                             blk_bs(extent_blk)->filename);
+        created_size += cur_size;
+        extent_idx++;
+        blk_unref(extent_blk);
     }
     /* generate descriptor file */
     desc = g_strdup_printf(desc_template,
                            g_random_int(),
                            parent_cid,
-                           fmt,
+                           vmdk_subformat_str(subformat),
                            parent_desc_line,
                            ext_desc_lines->str,
                            hw_version,
-                           total_size /
+                           size /
                                (int64_t)(63 * number_heads * BDRV_SECTOR_SIZE),
                            number_heads,
-                           adapter_type);
+                           qapi_enum_lookup(&BlockdevVmdkAdapterType_lookup,
+                                            adapter_type));
     desc_len = strlen(desc);
     /* the descriptor offset = 0x200 */
     if (!split && !flat) {
         desc_offset = 0x200;
-    } else {
-        ret = bdrv_create_file(filename, opts, &local_err);
-        if (ret < 0) {
-            error_propagate(errp, local_err);
-            goto exit;
-        }
     }
 
-    new_blk = blk_new_open(filename, NULL, NULL,
-                           BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
-                           &local_err);
-    if (new_blk == NULL) {
-        error_propagate(errp, local_err);
-        ret = -EIO;
-        goto exit;
-    }
-
-    blk_set_allow_write_beyond_eof(new_blk, true);
-
-    ret = blk_pwrite(new_blk, desc_offset, desc, desc_len, 0);
+    ret = blk_pwrite(blk, desc_offset, desc, desc_len, 0);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not write description");
         goto exit;
@@ -2138,12 +2147,148 @@  static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
     /* bdrv_pwrite write padding zeros to align to sector, we don't need that
      * for description file */
     if (desc_offset == 0) {
-        ret = blk_truncate(new_blk, desc_len, PREALLOC_MODE_OFF, errp);
+        ret = blk_truncate(blk, desc_len, PREALLOC_MODE_OFF, errp);
     }
 exit:
-    if (new_blk) {
-        blk_unref(new_blk);
+    if (blk) {
+        blk_unref(blk);
     }
+    g_free(desc);
+    g_free(parent_desc_line);
+    g_string_free(ext_desc_lines, true);
+    return ret;
+}
+
+typedef struct {
+    char *path;
+    char *prefix;
+    char *postfix;
+    QemuOpts *opts;
+} VMDKCreateOptsData;
+
+static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
+                                            bool flat, bool split, bool compress,
+                                            bool zeroed_grain, void *opaque,
+                                            Error **errp)
+{
+    BlockBackend *blk = NULL;
+    BlockDriverState *bs = NULL;
+    VMDKCreateOptsData *data = opaque;
+    char *ext_filename = NULL;
+    char *rel_filename = NULL;
+
+    if (idx == 0) {
+        rel_filename = g_strdup_printf("%s%s", data->prefix, data->postfix);
+    } else if (split) {
+        rel_filename = g_strdup_printf("%s-%c%03d%s",
+                                       data->prefix,
+                                       flat ? 'f' : 's', idx, data->postfix);
+    } else {
+        assert(idx == 1);
+        rel_filename = g_strdup_printf("%s-flat%s", data->prefix, data->postfix);
+    }
+
+    ext_filename = g_strdup_printf("%s%s", data->path, rel_filename);
+    g_free(rel_filename);
+
+    if (vmdk_create_extent(ext_filename, size,
+                           flat, compress, zeroed_grain, &blk, data->opts,
+                           errp)) {
+        goto exit;
+    }
+    bdrv_unref(bs);
+exit:
+    g_free(ext_filename);
+    return blk;
+}
+
+static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
+                                            Error **errp)
+{
+    Error *local_err = NULL;
+    char *desc = NULL;
+    int64_t total_size = 0;
+    char *adapter_type = NULL;
+    BlockdevVmdkAdapterType adapter_type_enum;
+    char *backing_file = NULL;
+    char *hw_version = NULL;
+    char *fmt = NULL;
+    BlockdevVmdkSubformat subformat;
+    int ret = 0;
+    char *path = g_malloc0(PATH_MAX);
+    char *prefix = g_malloc0(PATH_MAX);
+    char *postfix = g_malloc0(PATH_MAX);
+    char *desc_line = g_malloc0(BUF_SIZE);
+    char *ext_filename = g_malloc0(PATH_MAX);
+    char *desc_filename = g_malloc0(PATH_MAX);
+    char *parent_desc_line = g_malloc0(BUF_SIZE);
+    bool zeroed_grain;
+    bool compat6;
+    int i;
+    VMDKCreateOptsData data;
+
+    if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
+        ret = -EINVAL;
+        goto exit;
+    }
+    /* Read out options */
+    total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
+                          BDRV_SECTOR_SIZE);
+    adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
+    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
+    hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION);
+    compat6 = qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false);
+    if (strcmp(hw_version, "undefined") == 0) {
+        g_free(hw_version);
+        hw_version = g_strdup("4");
+    }
+    fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
+    zeroed_grain = qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false);
+
+    if (adapter_type) {
+        for (i = 0; i < strlen(adapter_type); ++i) {
+            adapter_type[i] = qemu_tolower(adapter_type[i]);
+        }
+        adapter_type_enum = qapi_enum_parse_full(&BlockdevVmdkAdapterType_lookup,
+                                                 adapter_type,
+                                                 BLOCKDEV_VMDK_ADAPTER_TYPE_IDE,
+                                                 true,
+                                                 &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            ret = -EINVAL;
+            goto exit;
+        }
+    } else {
+        adapter_type_enum = BLOCKDEV_VMDK_ADAPTER_TYPE_IDE;
+    }
+
+    if (!fmt) {
+        /* Default format to monolithicSparse */
+        subformat = BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE;
+    } else {
+        subformat = qapi_enum_parse_full(&BlockdevVmdkSubformat_lookup,
+                                         fmt,
+                                         BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE,
+                                         true,
+                                         &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            ret = -EINVAL;
+            goto exit;
+        }
+    }
+    data = (VMDKCreateOptsData){
+        .prefix = prefix,
+        .postfix = postfix,
+        .path = path,
+        .opts = opts,
+    };
+    ret = vmdk_co_do_create(total_size, subformat, adapter_type_enum,
+                            backing_file, hw_version, compat6, zeroed_grain,
+                            vmdk_co_create_opts_cb, &data, errp);
+
+exit:
     g_free(adapter_type);
     g_free(backing_file);
     g_free(hw_version);
@@ -2156,7 +2301,84 @@  exit:
     g_free(ext_filename);
     g_free(desc_filename);
     g_free(parent_desc_line);
-    g_string_free(ext_desc_lines, true);
+    return ret;
+}
+
+static BlockBackend *vmdk_co_create_cb(int64_t size, int idx,
+                                       bool flat, bool split, bool compress,
+                                       bool zeroed_grain, void *opaque,
+                                       Error **errp)
+{
+    int ret;
+    BlockDriverState *bs;
+    BlockBackend *blk;
+    BlockdevCreateOptionsVmdk *opts = opaque;
+
+    if (idx == 0) {
+        bs = bdrv_open_blockdev_ref(opts->file, errp);
+    } else {
+        int i;
+        BlockdevRefList *list = opts->extents;
+        for (i = 1; i < idx; i++) {
+            if (!list || !list->next) {
+                error_setg(errp, "Extent [%d] not specified", i);
+                return NULL;
+            }
+            list = list->next;
+        }
+        if (!list) {
+            error_setg(errp, "Extent [%d] not specified", idx - 1);
+            return NULL;
+        }
+        bs = bdrv_open_blockdev_ref(list->value, errp);
+    }
+    if (!bs) {
+        return NULL;
+    }
+    blk = blk_new(BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE,
+                  BLK_PERM_ALL);
+    if (blk_insert_bs(blk, bs, errp)) {
+        bdrv_unref(bs);
+        return NULL;
+    }
+    blk_set_allow_write_beyond_eof(blk, true);
+    bdrv_unref(bs);
+
+    ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain, errp);
+    if (ret) {
+        blk_unref(blk);
+        blk = NULL;
+    }
+    return blk;
+}
+
+static int coroutine_fn vmdk_co_create(BlockdevCreateOptions *create_options,
+                                       Error **errp)
+{
+    int ret;
+    BlockdevCreateOptionsVmdk *opts;
+
+    opts = &create_options->u.vmdk;
+
+    /* Validate options */
+    if (!QEMU_IS_ALIGNED(opts->size, BDRV_SECTOR_SIZE)) {
+        error_setg(errp, "Image size must be a multiple of 512 bytes");
+        ret = -EINVAL;
+        goto out;
+    }
+
+    ret = vmdk_co_do_create(opts->size,
+                            opts->subformat,
+                            opts->adapter_type,
+                            opts->backing_file,
+                            opts->hwversion,
+                            false,
+                            opts->zeroed_grain,
+                            vmdk_co_create_cb,
+                            opts, errp);
+    return ret;
+
+out:
     return ret;
 }
 
@@ -2424,6 +2646,7 @@  static BlockDriver bdrv_vmdk = {
     .bdrv_co_pwrite_zeroes        = vmdk_co_pwrite_zeroes,
     .bdrv_close                   = vmdk_close,
     .bdrv_co_create_opts          = vmdk_co_create_opts,
+    .bdrv_co_create               = vmdk_co_create,
     .bdrv_co_flush_to_disk        = vmdk_co_flush,
     .bdrv_co_block_status         = vmdk_co_block_status,
     .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c50517bff3..df3903b54d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3855,6 +3855,71 @@ 
             'size':             'size',
             '*cluster-size' :   'size' } }
 
+##
+# @BlockdevVmdkSubformat:
+#
+# Subformat options for VMDK images
+#
+# @monolithicsparse: Single file image with sparse cluster allocation
+# @monolithicflat: Single flat data image and a descriptor file
+# @twogbmaxextentsparse: Data is split into 2GB (per virtual LBA) sparse extent
+#                        files, in addition to a descriptor file
+# @twogbmaxextentflat: Data is split into 2GB (per virtual LBA) flat extent
+#                        files, in addition to a descriptor file
+# @streamoptimized: Single file image sparse cluster allocation, optimized for
+#                   streaming over network.
+#
+# Since: 2.13
+##
+{ 'enum': 'BlockdevVmdkSubformat',
+  'data': [ 'monolithicsparse', 'monolithicflat', 'twogbmaxextentsparse',
+            'twogbmaxextentflat', 'streamoptimized'] }
+
+##
+# @BlockdevVmdkAdapterType:
+#
+# Adapter type info for VMDK images
+#
+# Since: 2.13
+##
+{ 'enum': 'BlockdevVmdkAdapterType',
+  'data': [ 'ide', 'buslogic', 'lsilogic', 'legacyesx'] }
+
+##
+# @BlockdevCreateOptionsVmdk:
+#
+# Driver specific image creation options for VMDK.
+#
+# @file         Where to store the new image file. This refers to the image
+#               file for monolithcSparse and streamOptimized format, or the
+#               descriptor file for other formats.
+# @size         Size of the virtual disk in bytes
+# @extents      Where to store the data extents. Required for monolithcflat,
+#               twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For
+#               monolithicflat, only one entry is required; for
+#               twoGbMaxExtent* formats, the number of entries required is
+#               calculated as extent_number = virtual_size / 2GB.
+# @subformat    The subformat of the VMDK image. Default: "monolithicsparse".
+# @backing-file The path of backing file. Default: no backing file is used.
+# @adapter-type The adapter type used to fill in the descriptor. Default: ide.
+# @hwversion    Hardware version. The meaningful options are "4" or "6".
+#               Defaulted to "4".
+# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats.
+#               Default: false.
+#
+# Since: 2.13
+##
+{ 'struct': 'BlockdevCreateOptionsVmdk',
+  'data': { 'file':             'BlockdevRef',
+            'size':             'size',
+            '*extents':          ['BlockdevRef'],
+            '*subformat':       'BlockdevVmdkSubformat',
+            '*backing-file':    'str',
+            '*adapter-type':    'BlockdevVmdkAdapterType',
+            '*hwversion':       'str',
+            '*zeroed-grain':    'bool' } }
+
+
 ##
 # @SheepdogRedundancyType:
 #
@@ -4078,7 +4143,7 @@ 
       'throttle':       'BlockdevCreateNotSupported',
       'vdi':            'BlockdevCreateOptionsVdi',
       'vhdx':           'BlockdevCreateOptionsVhdx',
-      'vmdk':           'BlockdevCreateNotSupported',
+      'vmdk':           'BlockdevCreateOptionsVmdk',
       'vpc':            'BlockdevCreateOptionsVpc',
       'vvfat':          'BlockdevCreateNotSupported',
       'vxhs':           'BlockdevCreateNotSupported'