[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 3/5] vmdk: Implement .bdrv_co_creat
|
From: |
Markus Armbruster |
|
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 3/5] vmdk: Implement .bdrv_co_create callback |
|
Date: |
Wed, 09 May 2018 20:59:40 +0200 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
Fam Zheng <address@hidden> writes:
> On Wed, 05/09 14:41, Markus Armbruster wrote:
>> Beware, I'm only looking at QAPI-related aspects.
>>
>> Fam Zheng <address@hidden> 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 <address@hidden>
>> > ---
>> > 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
- [Qemu-block] [PATCH 0/5] vmdk: Implement x-blockdev-create, Fam Zheng, 2018/05/09
- [Qemu-block] [PATCH 4/5] iotests: Filter cid numbers in VMDK extent info, Fam Zheng, 2018/05/09
- [Qemu-block] [PATCH 5/5] iotests: Add VMDK tests for blockdev-create, Fam Zheng, 2018/05/09
- Re: [Qemu-block] [Qemu-devel] [PATCH 0/5] vmdk: Implement x-blockdev-create, no-reply, 2018/05/09
- Re: [Qemu-block] [Qemu-devel] [PATCH 0/5] vmdk: Implement x-blockdev-create, no-reply, 2018/05/09