On Wed, 05/09 14:41, Markus Armbruster wrote:
> Beware, I'm only looking at QAPI-related aspects.
>
> Fam Zheng <f...@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 <f...@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
