This is a reasonably careful review of the QAPI-related parts, but more
of an eye-over for the remainder.
Kevin Wolf <kw...@redhat.com> writes:
> From: Fam Zheng <f...@redhat.com>
>
> This makes VMDK support blockdev-create. The implementation reuses the
> image creation code in vmdk_co_create_opts which now acceptes a callback
> pointer to "retrieve" BlockBackend pointers from the caller. This way we
> separate the logic between file/extent acquisition and initialization.
>
> The QAPI command parameters are mostly the same as the old create_opts
> except the dropped legacy @compat6 switch, which is redundant with
> @hwversion.
>
> Signed-off-by: Fam Zheng <f...@redhat.com>
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
> qapi/block-core.json | 70 +++++++
> qapi/qapi-schema.json | 1 +
> block/vmdk.c | 452 ++++++++++++++++++++++++++++++------------
> 3 files changed, 396 insertions(+), 127 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d4fe710836..4778f88dd8 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4021,6 +4021,75 @@
> '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: 4.0
> +##
> +{ 'enum': 'BlockdevVmdkSubformat',
> + 'data': [ 'monolithicSparse', 'monolithicFlat', 'twoGbMaxExtentSparse',
> + 'twoGbMaxExtentFlat', 'streamOptimized'] }
Don't conform to the QAPI rules on names "to match VMDK spec spellings"
(see qapi-schema.json below). We discussed this in review of v1.
PRO: matches the VMDK spec (whatever that's worth), keeps the code
simple.
CON: the non-conforming names become part of the stable QMP interface,
in the argument of command blockdev-create.
I don't like the CON, but I'm willing to tolerate it in the name of
simplicity.
> +
> +##
> +# @BlockdevVmdkAdapterType:
> +#
> +# Adapter type info for VMDK images
> +#
> +# Since: 4.0
> +##
> +{ 'enum': 'BlockdevVmdkAdapterType',
> + 'data': [ 'ide', 'buslogic', 'lsilogic', 'legacyesx'] }
May I have hyphens in the composite nouns? Hmm, might be the way they
are to match VMDK spec spellings or for backward compatibility. I guess
we'll see below.
> +
> +##
> +# @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
monolithicFlat
> +# twoGbMaxExtent* formats, the number of entries required is
> +# calculated as extent_number = virtual_size / 2GB.
Is it okay to supply more entries than required, or do I have to supply
exactly the right number?
> +# @subformat The subformat of the VMDK image. Default: "monolithicsparse".
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".
Default: "4"
> +# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats.
> +# Default: false.
> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'BlockdevCreateOptionsVmdk',
> + 'data': { 'file': 'BlockdevRef',
> + 'size': 'size',
> + '*extents': ['BlockdevRef'],
> + '*subformat': 'BlockdevVmdkSubformat',
> + '*backing-file': 'str',
> + '*adapter-type': 'BlockdevVmdkAdapterType',
> + '*hwversion': 'str',
> + '*zeroed-grain': 'bool' } }
@zeroed-grain is undocumented.
> +
> +
> ##
> # @SheepdogRedundancyType:
> #
> @@ -4215,6 +4284,7 @@
> 'ssh': 'BlockdevCreateOptionsSsh',
> 'vdi': 'BlockdevCreateOptionsVdi',
> 'vhdx': 'BlockdevCreateOptionsVhdx',
> + 'vmdk': 'BlockdevCreateOptionsVmdk',
> 'vpc': 'BlockdevCreateOptionsVpc'
> } }
>
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index 65b6dc2f6f..78e8bcd561 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -66,6 +66,7 @@
> 'ACPISlotType', # DIMM, visible through
> query-acpi-ospm-status
> 'CpuInfoMIPS', # PC, visible through query-cpu
> 'CpuInfoTricore', # PC, visible through query-cpu
> + 'BlockdevVmdkSubformat',# all members, to match VMDK spec spellings
> 'QapiErrorClass', # all members, visible through errors
> 'UuidInfo', # UUID, visible through query-uuid
> 'X86CPURegister32', # all members, visible indirectly through
> qom-get
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 32fc2c84b3..16f86457d7 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1932,33 +1932,56 @@ 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)
> +/*
> + * 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 *basename = g_path_get_basename(filename);
I like a blank line between declarations and statements.
> + g_string_append_printf(desc, extent_line_fmt,
> + DIV_ROUND_UP(size, BDRV_SECTOR_SIZE), basename);
> +
> + g_free(basename);
> +}
> +
> +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 idx = 0;
> - BlockBackend *new_blk = NULL;
> + int extent_idx;
> + BlockBackend *blk = NULL;
> 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"
> @@ -1982,71 +2005,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");
> - }
> - if (strcmp(hw_version, "undefined") == 0) {
> - g_free(hw_version);
> - hw_version = g_strdup("4");
> + hw_version = "6";
> }
> - 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;
> - }
Old code recognizes "legacyESX". New code recognizes "legacyesx". Bug
or feature?
> - 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");
[...]
