| Message ID | 20180509055802.28423-4-famz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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'
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
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
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'
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(-)