* [Qemu-devel] [PATCH v4 1/5] vmdk: Refactor vmdk_create_extent
2018-12-07 11:53 [Qemu-devel] [PATCH v4 0/5] vmdk: Implement blockdev-create Kevin Wolf
@ 2018-12-07 11:53 ` Kevin Wolf
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 2/5] vmdk: Implement .bdrv_co_create callback Kevin Wolf
` (4 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Kevin Wolf @ 2018-12-07 11:53 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, armbru, eblake, qemu-devel
From: Fam Zheng <famz@redhat.com>
The extracted vmdk_init_extent takes a BlockBackend object and
initializes the format metadata. It is the common part between "qemu-img
create" and "blockdev-create".
Add a "BlockBackend *pbb" parameter to vmdk_create_extent, to return the
opened BB to the caller in the next patch.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/vmdk.c | 69 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 43 insertions(+), 26 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 2c9e86d98f..32fc2c84b3 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1741,35 +1741,17 @@ static int coroutine_fn vmdk_co_pwrite_zeroes(BlockDriverState *bs,
return ret;
}
-static int vmdk_create_extent(const char *filename, int64_t filesize,
- bool flat, bool compress, bool zeroed_grain,
- QemuOpts *opts, Error **errp)
+static int vmdk_init_extent(BlockBackend *blk,
+ int64_t filesize, bool flat,
+ bool compress, bool zeroed_grain,
+ Error **errp)
{
int ret, i;
- BlockBackend *blk = NULL;
VMDK4Header header;
- Error *local_err = NULL;
uint32_t tmp, magic, grains, gd_sectors, gt_size, gt_count;
uint32_t *gd_buf = NULL;
int gd_buf_size;
- ret = bdrv_create_file(filename, opts, &local_err);
- if (ret < 0) {
- error_propagate(errp, local_err);
- goto exit;
- }
-
- blk = blk_new_open(filename, NULL, NULL,
- BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
- &local_err);
- if (blk == NULL) {
- error_propagate(errp, local_err);
- ret = -EIO;
- goto exit;
- }
-
- blk_set_allow_write_beyond_eof(blk, true);
-
if (flat) {
ret = blk_truncate(blk, filesize, PREALLOC_MODE_OFF, errp);
goto exit;
@@ -1863,15 +1845,50 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
gd_buf, gd_buf_size, 0);
if (ret < 0) {
error_setg(errp, QERR_IO_ERROR);
- goto exit;
}
ret = 0;
+exit:
+ g_free(gd_buf);
+ return ret;
+}
+
+static int vmdk_create_extent(const char *filename, int64_t filesize,
+ bool flat, bool compress, bool zeroed_grain,
+ BlockBackend **pbb,
+ QemuOpts *opts, Error **errp)
+{
+ int ret;
+ BlockBackend *blk = NULL;
+ Error *local_err = NULL;
+
+ ret = bdrv_create_file(filename, opts, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
+ goto exit;
+ }
+
+ blk = blk_new_open(filename, NULL, NULL,
+ BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
+ &local_err);
+ if (blk == NULL) {
+ error_propagate(errp, local_err);
+ ret = -EIO;
+ goto exit;
+ }
+
+ blk_set_allow_write_beyond_eof(blk, true);
+
+ ret = vmdk_init_extent(blk, filesize, flat, compress, zeroed_grain, errp);
exit:
if (blk) {
- blk_unref(blk);
+ if (pbb) {
+ *pbb = blk;
+ } else {
+ blk_unref(blk);
+ blk = NULL;
+ }
}
- g_free(gd_buf);
return ret;
}
@@ -2094,7 +2111,7 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
snprintf(ext_filename, PATH_MAX, "%s%s", path, desc_filename);
if (vmdk_create_extent(ext_filename, size,
- flat, compress, zeroed_grain, opts, errp)) {
+ flat, compress, zeroed_grain, NULL, opts, errp)) {
ret = -EINVAL;
goto exit;
}
--
2.19.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Qemu-devel] [PATCH v4 2/5] vmdk: Implement .bdrv_co_create callback
2018-12-07 11:53 [Qemu-devel] [PATCH v4 0/5] vmdk: Implement blockdev-create Kevin Wolf
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 1/5] vmdk: Refactor vmdk_create_extent Kevin Wolf
@ 2018-12-07 11:53 ` Kevin Wolf
2018-12-07 13:01 ` Markus Armbruster
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 3/5] iotests: Filter cid numbers in VMDK extent info Kevin Wolf
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Kevin Wolf @ 2018-12-07 11:53 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, armbru, eblake, qemu-devel
From: Fam Zheng <famz@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 <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 70 +++++++
qapi/qapi-schema.json | 16 +-
block/vmdk.c | 448 ++++++++++++++++++++++++++++++------------
3 files changed, 400 insertions(+), 134 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d4fe710836..0793550cf2 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'] }
+
+##
+# @BlockdevVmdkAdapterType:
+#
+# Adapter type info for VMDK images
+#
+# Since: 4.0
+##
+{ '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".
+# 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' } }
+
+
##
# @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..339f9b315b 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -63,13 +63,15 @@
'query-tpm-types',
'ringbuf-read' ],
'name-case-whitelist': [
- 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status
- 'CpuInfoMIPS', # PC, visible through query-cpu
- 'CpuInfoTricore', # PC, visible through query-cpu
- 'QapiErrorClass', # all members, visible through errors
- 'UuidInfo', # UUID, visible through query-uuid
- 'X86CPURegister32', # all members, visible indirectly through qom-get
- 'q_obj_CpuInfo-base' # CPU, visible through query-cpu
+ '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
+ 'BlockdevVmdkAdapterType', # legacyESX, 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
+ 'q_obj_CpuInfo-base' # CPU, visible through query-cpu
] } }
# Documentation generated with qapi-gen.py is in source order, with
diff --git a/block/vmdk.c b/block/vmdk.c
index 32fc2c84b3..5a162ee85c 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);
+
+ 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;
- }
- 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");
@@ -2058,10 +2045,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) {
@@ -2071,106 +2082,211 @@ 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,
+ BlockdevVmdkSubformat_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);
+ BlockdevVmdkAdapterType_str(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);
+ }
+
+ ret = blk_pwrite(blk, desc_offset, desc, desc_len, 0);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Could not write description");
+ goto exit;
+ }
+ /* bdrv_pwrite write padding zeros to align to sector, we don't need that
+ * for description file */
+ if (desc_offset == 0) {
+ ret = blk_truncate(blk, desc_len, PREALLOC_MODE_OFF, errp);
if (ret < 0) {
- error_propagate(errp, local_err);
goto exit;
}
}
+ ret = 0;
+exit:
+ if (blk) {
+ blk_unref(blk);
+ }
+ g_free(desc);
+ g_free(parent_desc_line);
+ g_string_free(ext_desc_lines, true);
+ return ret;
+}
- 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;
+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;
+}
- blk_set_allow_write_beyond_eof(new_blk, true);
+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;
+ VMDKCreateOptsData data;
- ret = blk_pwrite(new_blk, desc_offset, desc, desc_len, 0);
- if (ret < 0) {
- error_setg_errno(errp, -ret, "Could not write description");
+ if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
+ ret = -EINVAL;
goto exit;
}
- /* 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);
+ /* 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");
}
-exit:
- if (new_blk) {
- blk_unref(new_blk);
+ 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) {
+ adapter_type_enum = qapi_enum_parse(&BlockdevVmdkAdapterType_lookup,
+ adapter_type,
+ BLOCKDEV_VMDK_ADAPTER_TYPE_IDE,
+ &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(&BlockdevVmdkSubformat_lookup,
+ fmt,
+ BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE,
+ &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);
@@ -2183,7 +2299,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;
}
@@ -2451,6 +2644,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,
--
2.19.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v4 2/5] vmdk: Implement .bdrv_co_create callback
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 2/5] vmdk: Implement .bdrv_co_create callback Kevin Wolf
@ 2018-12-07 13:01 ` Markus Armbruster
2018-12-07 13:04 ` Markus Armbruster
0 siblings, 1 reply; 19+ messages in thread
From: Markus Armbruster @ 2018-12-07 13:01 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, mreitz
Kevin Wolf <kwolf@redhat.com> writes:
> From: Fam Zheng <famz@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 <famz@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 70 +++++++
> qapi/qapi-schema.json | 16 +-
> block/vmdk.c | 448 ++++++++++++++++++++++++++++++------------
> 3 files changed, 400 insertions(+), 134 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d4fe710836..0793550cf2 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'] }
> +
> +##
> +# @BlockdevVmdkAdapterType:
> +#
> +# Adapter type info for VMDK images
> +#
> +# Since: 4.0
> +##
> +{ '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.
Doesn't quite spell out that the number of extents has to match
exactly. I'm not sure I care.
> +# @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".
> +# 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' } }
> +
> +
> ##
> # @SheepdogRedundancyType:
> #
> @@ -4215,6 +4284,7 @@
> 'ssh': 'BlockdevCreateOptionsSsh',
> 'vdi': 'BlockdevCreateOptionsVdi',
> 'vhdx': 'BlockdevCreateOptionsVhdx',
> + 'vmdk': 'BlockdevCreateOptionsVmdk',
> 'vpc': 'BlockdevCreateOptionsVpc'
> } }
>
[...]
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v4 2/5] vmdk: Implement .bdrv_co_create callback
2018-12-07 13:01 ` Markus Armbruster
@ 2018-12-07 13:04 ` Markus Armbruster
0 siblings, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2018-12-07 13:04 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, qemu-block, qemu-devel, mreitz
Markus Armbruster <armbru@redhat.com> writes:
> Kevin Wolf <kwolf@redhat.com> writes:
>
>> From: Fam Zheng <famz@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 <famz@redhat.com>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>> qapi/block-core.json | 70 +++++++
>> qapi/qapi-schema.json | 16 +-
>> block/vmdk.c | 448 ++++++++++++++++++++++++++++++------------
>> 3 files changed, 400 insertions(+), 134 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index d4fe710836..0793550cf2 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
[...]
>> +##
>> +# @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.
>
> Doesn't quite spell out that the number of extents has to match
> exactly. I'm not sure I care.
Nevermind, you address that in PATCH 5.
[...]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 3/5] iotests: Filter cid numbers in VMDK extent info
2018-12-07 11:53 [Qemu-devel] [PATCH v4 0/5] vmdk: Implement blockdev-create Kevin Wolf
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 1/5] vmdk: Refactor vmdk_create_extent Kevin Wolf
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 2/5] vmdk: Implement .bdrv_co_create callback Kevin Wolf
@ 2018-12-07 11:53 ` Kevin Wolf
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-create Kevin Wolf
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Kevin Wolf @ 2018-12-07 11:53 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, armbru, eblake, qemu-devel
From: Fam Zheng <famz@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/qemu-iotests/common.filter | 1 +
tests/qemu-iotests/iotests.py | 1 +
2 files changed, 2 insertions(+)
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 2031e353a5..1aa7d57140 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -165,6 +165,7 @@ _filter_img_info()
-e "/table_size: [0-9]\\+/d" \
-e "/compat: '[^']*'/d" \
-e "/compat6: \\(on\\|off\\)/d" \
+ -e "s/cid: [0-9]\+/cid: XXXXXXXXXX/" \
-e "/static: \\(on\\|off\\)/d" \
-e "/zeroed_grain: \\(on\\|off\\)/d" \
-e "/subformat: '[^']*'/d" \
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index d537538ba0..4142937239 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -248,6 +248,7 @@ def filter_img_info(output, filename):
.replace(imgfmt, 'IMGFMT')
line = re.sub('iters: [0-9]+', 'iters: XXX', line)
line = re.sub('uuid: [-a-f0-9]+', 'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX', line)
+ line = re.sub('cid: [0-9]+', 'cid: XXXXXXXXXX', line)
lines.append(line)
return '\n'.join(lines)
--
2.19.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-create
2018-12-07 11:53 [Qemu-devel] [PATCH v4 0/5] vmdk: Implement blockdev-create Kevin Wolf
` (2 preceding siblings ...)
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 3/5] iotests: Filter cid numbers in VMDK extent info Kevin Wolf
@ 2018-12-07 11:53 ` Kevin Wolf
2018-12-07 13:02 ` Markus Armbruster
2018-12-07 13:12 ` Markus Armbruster
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 5/5] vmdk: Reject excess extents in blockdev-create Kevin Wolf
2018-12-07 12:56 ` [Qemu-devel] [PATCH v4 0/5] vmdk: Implement blockdev-create no-reply
5 siblings, 2 replies; 19+ messages in thread
From: Kevin Wolf @ 2018-12-07 11:53 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, armbru, eblake, qemu-devel
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/qemu-iotests/237 | 233 +++++++++++++++++++++++++
tests/qemu-iotests/237.out | 347 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 581 insertions(+)
create mode 100755 tests/qemu-iotests/237
create mode 100644 tests/qemu-iotests/237.out
diff --git a/tests/qemu-iotests/237 b/tests/qemu-iotests/237
new file mode 100755
index 0000000000..e04a1ac6be
--- /dev/null
+++ b/tests/qemu-iotests/237
@@ -0,0 +1,233 @@
+#!/usr/bin/env python
+#
+# Test vmdk and file image creation
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# Creator/Owner: Kevin Wolf <kwolf@redhat.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import iotests
+from iotests import imgfmt
+
+iotests.verify_image_format(supported_fmts=['vmdk'])
+
+def blockdev_create(vm, options):
+ result = vm.qmp_log('blockdev-create', job_id='job0', options=options)
+
+ if 'return' in result:
+ assert result['return'] == {}
+ vm.run_job('job0')
+ iotests.log("")
+
+with iotests.FilePath('t.vmdk') as disk_path, \
+ iotests.FilePath('t.vmdk.1') as extent1_path, \
+ iotests.FilePath('t.vmdk.2') as extent2_path, \
+ iotests.FilePath('t.vmdk.3') as extent3_path, \
+ iotests.VM() as vm:
+
+ #
+ # Successful image creation (defaults)
+ #
+ iotests.log("=== Successful image creation (defaults) ===")
+ iotests.log("")
+
+ size = 5 * 1024 * 1024 * 1024
+
+ vm.launch()
+ blockdev_create(vm, { 'driver': 'file',
+ 'filename': disk_path,
+ 'size': 0 })
+
+ vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
+ node_name='imgfile')
+
+ blockdev_create(vm, { 'driver': imgfmt,
+ 'file': 'imgfile',
+ 'size': size })
+ vm.shutdown()
+
+ iotests.img_info_log(disk_path)
+
+ #
+ # Successful image creation (inline blockdev-add, explicit defaults)
+ #
+ iotests.log("=== Successful image creation (inline blockdev-add, explicit defaults) ===")
+ iotests.log("")
+
+ # Choose a different size to show that we got a new image
+ size = 64 * 1024 * 1024
+
+ vm.launch()
+ blockdev_create(vm, { 'driver': 'file',
+ 'filename': disk_path,
+ 'size': 0 })
+
+ blockdev_create(vm, { 'driver': imgfmt,
+ 'file': {
+ 'driver': 'file',
+ 'filename': disk_path,
+ },
+ 'size': size,
+ 'extents': [],
+ 'subformat': 'monolithicSparse',
+ 'adapter-type': 'ide',
+ 'hwversion': '4',
+ 'zeroed-grain': False })
+ vm.shutdown()
+
+ iotests.img_info_log(disk_path)
+
+ #
+ # Successful image creation (non-default options)
+ #
+ iotests.log("=== Successful image creation (with non-default options) ===")
+ iotests.log("")
+
+ # Choose a different size to show that we got a new image
+ size = 32 * 1024 * 1024
+
+ vm.launch()
+ blockdev_create(vm, { 'driver': 'file',
+ 'filename': disk_path,
+ 'size': 0 })
+
+ blockdev_create(vm, { 'driver': imgfmt,
+ 'file': {
+ 'driver': 'file',
+ 'filename': disk_path,
+ },
+ 'size': size,
+ 'extents': [],
+ 'subformat': 'monolithicSparse',
+ 'adapter-type': 'buslogic',
+ 'zeroed-grain': True })
+ vm.shutdown()
+
+ iotests.img_info_log(disk_path)
+
+ #
+ # Invalid BlockdevRef
+ #
+ iotests.log("=== Invalid BlockdevRef ===")
+ iotests.log("")
+
+ vm.launch()
+ blockdev_create(vm, { 'driver': imgfmt,
+ 'file': "this doesn't exist",
+ 'size': size })
+ vm.shutdown()
+
+ #
+ # Adapter types
+ #
+
+ iotests.log("=== Adapter types ===")
+ iotests.log("")
+
+ vm.add_blockdev('driver=file,filename=%s,node-name=node0' % (disk_path))
+
+ # Valid
+ iotests.log("== Valid adapter types ==")
+ iotests.log("")
+
+ vm.launch()
+ for adapter_type in [ 'ide', 'buslogic', 'lsilogic', 'legacyESX' ]:
+ blockdev_create(vm, { 'driver': imgfmt,
+ 'file': 'node0',
+ 'size': size,
+ 'adapter-type': adapter_type })
+ vm.shutdown()
+
+ # Invalid
+ iotests.log("== Invalid adapter types ==")
+ iotests.log("")
+
+ vm.launch()
+ for adapter_type in [ 'foo', 'IDE', 'legacyesx', 1 ]:
+ blockdev_create(vm, { 'driver': imgfmt,
+ 'file': 'node0',
+ 'size': size,
+ 'adapter-type': adapter_type })
+ vm.shutdown()
+
+ #
+ # Other subformats
+ #
+ iotests.log("=== Other subformats ===")
+ iotests.log("")
+
+ for path in [ extent1_path, extent2_path, extent3_path ]:
+ msg = iotests.qemu_img_pipe('create', '-f', imgfmt, path, '0')
+ iotests.log(msg, [iotests.filter_testfiles])
+
+ vm.add_blockdev('driver=file,filename=%s,node-name=ext1' % (extent1_path))
+ vm.add_blockdev('driver=file,filename=%s,node-name=ext2' % (extent2_path))
+ vm.add_blockdev('driver=file,filename=%s,node-name=ext3' % (extent3_path))
+
+ # Missing extent
+ iotests.log("== Missing extent ==")
+ iotests.log("")
+
+ vm.launch()
+ blockdev_create(vm, { 'driver': imgfmt,
+ 'file': 'node0',
+ 'size': size,
+ 'subformat': 'monolithicFlat' })
+ vm.shutdown()
+
+ # Correct extent
+ iotests.log("== Correct extent ==")
+ iotests.log("")
+
+ vm.launch()
+ blockdev_create(vm, { 'driver': imgfmt,
+ 'file': 'node0',
+ 'size': size,
+ 'subformat': 'monolithicFlat',
+ 'extents': ['ext1'] })
+ vm.shutdown()
+
+ # Extra extent
+ iotests.log("== Extra extent ==")
+ iotests.log("")
+
+ vm.launch()
+ blockdev_create(vm, { 'driver': imgfmt,
+ 'file': 'node0',
+ 'size': 512,
+ 'subformat': 'monolithicFlat',
+ 'extents': ['ext1', 'ext2', 'ext3'] })
+ vm.shutdown()
+
+ # Split formats
+ iotests.log("== Split formats ==")
+ iotests.log("")
+
+ for size in [ 512, 1073741824, 2147483648, 5368709120 ]:
+ for subfmt in [ 'twoGbMaxExtentFlat', 'twoGbMaxExtentSparse' ]:
+ iotests.log("= %s %d =" % (subfmt, size))
+ iotests.log("")
+
+ vm.launch()
+ blockdev_create(vm, { 'driver': imgfmt,
+ 'file': 'node0',
+ 'size': size,
+ 'subformat': subfmt,
+ 'extents': ['ext1', 'ext2', 'ext3'] })
+ vm.shutdown()
+
+ iotests.img_info_log(disk_path)
diff --git a/tests/qemu-iotests/237.out b/tests/qemu-iotests/237.out
new file mode 100644
index 0000000000..1aa9aad349
--- /dev/null
+++ b/tests/qemu-iotests/237.out
@@ -0,0 +1,347 @@
+=== Successful image creation (defaults) ===
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vmdk", "size": 0}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+{"execute": "blockdev-add", "arguments": {"driver": "file", "filename": "TEST_DIR/PID-t.vmdk", "node_name": "imgfile"}}
+{"return": {}}
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "file": "imgfile", "size": 5368709120}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 5.0G (5368709120 bytes)
+cluster_size: 65536
+Format specific information:
+ cid: XXXXXXXXXX
+ parent cid: XXXXXXXXXX
+ create type: monolithicSparse
+ extents:
+ [0]:
+ virtual size: 5368709120
+ filename: TEST_IMG
+ cluster size: 65536
+ format:
+
+=== Successful image creation (inline blockdev-add, explicit defaults) ===
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vmdk", "size": 0}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"adapter-type": "ide", "driver": "vmdk", "extents": [], "file": {"driver": "file", "filename": "TEST_DIR/PID-t.vmdk"}, "hwversion": "4", "size": 67108864, "subformat": "monolithicSparse", "zeroed-grain": false}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+cluster_size: 65536
+Format specific information:
+ cid: XXXXXXXXXX
+ parent cid: XXXXXXXXXX
+ create type: monolithicSparse
+ extents:
+ [0]:
+ virtual size: 67108864
+ filename: TEST_IMG
+ cluster size: 65536
+ format:
+
+=== Successful image creation (with non-default options) ===
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vmdk", "size": 0}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"adapter-type": "buslogic", "driver": "vmdk", "extents": [], "file": {"driver": "file", "filename": "TEST_DIR/PID-t.vmdk"}, "size": 33554432, "subformat": "monolithicSparse", "zeroed-grain": true}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 32M (33554432 bytes)
+cluster_size: 65536
+Format specific information:
+ cid: XXXXXXXXXX
+ parent cid: XXXXXXXXXX
+ create type: monolithicSparse
+ extents:
+ [0]:
+ virtual size: 33554432
+ filename: TEST_IMG
+ cluster size: 65536
+ format:
+
+=== Invalid BlockdevRef ===
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "file": "this doesn't exist", "size": 33554432}}}
+{"return": {}}
+Job failed: Cannot find device=this doesn't exist nor node_name=this doesn't exist
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+=== Adapter types ===
+
+== Valid adapter types ==
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"adapter-type": "ide", "driver": "vmdk", "file": "node0", "size": 33554432}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"adapter-type": "buslogic", "driver": "vmdk", "file": "node0", "size": 33554432}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"adapter-type": "lsilogic", "driver": "vmdk", "file": "node0", "size": 33554432}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"adapter-type": "legacyESX", "driver": "vmdk", "file": "node0", "size": 33554432}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+== Invalid adapter types ==
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"adapter-type": "foo", "driver": "vmdk", "file": "node0", "size": 33554432}}}
+{"error": {"class": "GenericError", "desc": "Invalid parameter 'foo'"}}
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"adapter-type": "IDE", "driver": "vmdk", "file": "node0", "size": 33554432}}}
+{"error": {"class": "GenericError", "desc": "Invalid parameter 'IDE'"}}
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"adapter-type": "legacyesx", "driver": "vmdk", "file": "node0", "size": 33554432}}}
+{"error": {"class": "GenericError", "desc": "Invalid parameter 'legacyesx'"}}
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"adapter-type": 1, "driver": "vmdk", "file": "node0", "size": 33554432}}}
+{"error": {"class": "GenericError", "desc": "Invalid parameter type for 'options.adapter-type', expected: string"}}
+
+=== Other subformats ===
+
+Formatting 'TEST_DIR/PID-t.vmdk.1', fmt=vmdk size=0 compat6=off hwversion=undefined
+
+Formatting 'TEST_DIR/PID-t.vmdk.2', fmt=vmdk size=0 compat6=off hwversion=undefined
+
+Formatting 'TEST_DIR/PID-t.vmdk.3', fmt=vmdk size=0 compat6=off hwversion=undefined
+
+== Missing extent ==
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "file": "node0", "size": 33554432, "subformat": "monolithicFlat"}}}
+{"return": {}}
+Job failed: Extent [0] not specified
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+== Correct extent ==
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 33554432, "subformat": "monolithicFlat"}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+== Extra extent ==
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 512, "subformat": "monolithicFlat"}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+== Split formats ==
+
+= twoGbMaxExtentFlat 512 =
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentFlat"}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 512 (512 bytes)
+Format specific information:
+ cid: XXXXXXXXXX
+ parent cid: XXXXXXXXXX
+ create type: twoGbMaxExtentFlat
+ extents:
+ [0]:
+ virtual size: 512
+ filename: TEST_IMG.1
+ format: FLAT
+
+= twoGbMaxExtentSparse 512 =
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentSparse"}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 512 (512 bytes)
+cluster_size: 65536
+Format specific information:
+ cid: XXXXXXXXXX
+ parent cid: XXXXXXXXXX
+ create type: twoGbMaxExtentSparse
+ extents:
+ [0]:
+ virtual size: 512
+ filename: TEST_IMG.1
+ cluster size: 65536
+ format: SPARSE
+
+= twoGbMaxExtentFlat 1073741824 =
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentFlat"}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 1.0G (1073741824 bytes)
+Format specific information:
+ cid: XXXXXXXXXX
+ parent cid: XXXXXXXXXX
+ create type: twoGbMaxExtentFlat
+ extents:
+ [0]:
+ virtual size: 1073741824
+ filename: TEST_IMG.1
+ format: FLAT
+
+= twoGbMaxExtentSparse 1073741824 =
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentSparse"}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 1.0G (1073741824 bytes)
+cluster_size: 65536
+Format specific information:
+ cid: XXXXXXXXXX
+ parent cid: XXXXXXXXXX
+ create type: twoGbMaxExtentSparse
+ extents:
+ [0]:
+ virtual size: 1073741824
+ filename: TEST_IMG.1
+ cluster size: 65536
+ format: SPARSE
+
+= twoGbMaxExtentFlat 2147483648 =
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 2147483648, "subformat": "twoGbMaxExtentFlat"}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 2.0G (2147483648 bytes)
+Format specific information:
+ cid: XXXXXXXXXX
+ parent cid: XXXXXXXXXX
+ create type: twoGbMaxExtentFlat
+ extents:
+ [0]:
+ virtual size: 2147483648
+ filename: TEST_IMG.1
+ format: FLAT
+
+= twoGbMaxExtentSparse 2147483648 =
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 2147483648, "subformat": "twoGbMaxExtentSparse"}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 2.0G (2147483648 bytes)
+cluster_size: 65536
+Format specific information:
+ cid: XXXXXXXXXX
+ parent cid: XXXXXXXXXX
+ create type: twoGbMaxExtentSparse
+ extents:
+ [0]:
+ virtual size: 2147483648
+ filename: TEST_IMG.1
+ cluster size: 65536
+ format: SPARSE
+
+= twoGbMaxExtentFlat 5368709120 =
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 5368709120, "subformat": "twoGbMaxExtentFlat"}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 5.0G (5368709120 bytes)
+Format specific information:
+ cid: XXXXXXXXXX
+ parent cid: XXXXXXXXXX
+ create type: twoGbMaxExtentFlat
+ extents:
+ [0]:
+ virtual size: 2147483648
+ filename: TEST_IMG.1
+ format: FLAT
+ [1]:
+ virtual size: 2147483648
+ filename: TEST_IMG.2
+ format: FLAT
+ [2]:
+ virtual size: 1073741824
+ filename: TEST_IMG.3
+ format: FLAT
+
+= twoGbMaxExtentSparse 5368709120 =
+
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 5368709120, "subformat": "twoGbMaxExtentSparse"}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 5.0G (5368709120 bytes)
+cluster_size: 65536
+Format specific information:
+ cid: XXXXXXXXXX
+ parent cid: XXXXXXXXXX
+ create type: twoGbMaxExtentSparse
+ extents:
+ [0]:
+ virtual size: 2147483648
+ filename: TEST_IMG.1
+ cluster size: 65536
+ format: SPARSE
+ [1]:
+ virtual size: 2147483648
+ filename: TEST_IMG.2
+ cluster size: 65536
+ format: SPARSE
+ [2]:
+ virtual size: 1073741824
+ filename: TEST_IMG.3
+ cluster size: 65536
+ format: SPARSE
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 61a6d98ebd..c4de595f29 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -233,3 +233,4 @@
233 auto quick
234 auto quick migration
235 auto quick
+237 rw auto quick
--
2.19.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-create
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-create Kevin Wolf
@ 2018-12-07 13:02 ` Markus Armbruster
2018-12-07 13:12 ` Markus Armbruster
1 sibling, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2018-12-07 13:02 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, mreitz
Kevin Wolf <kwolf@redhat.com> writes:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> tests/qemu-iotests/237 | 233 +++++++++++++++++++++++++
> tests/qemu-iotests/237.out | 347 +++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/group | 1 +
> 3 files changed, 581 insertions(+)
> create mode 100755 tests/qemu-iotests/237
> create mode 100644 tests/qemu-iotests/237.out
>
> diff --git a/tests/qemu-iotests/237 b/tests/qemu-iotests/237
> new file mode 100755
> index 0000000000..e04a1ac6be
> --- /dev/null
> +++ b/tests/qemu-iotests/237
> @@ -0,0 +1,233 @@
> +#!/usr/bin/env python
> +#
> +# Test vmdk and file image creation
> +#
> +# Copyright (C) 2018 Red Hat, Inc.
> +#
> +# Creator/Owner: Kevin Wolf <kwolf@redhat.com>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import iotests
> +from iotests import imgfmt
> +
> +iotests.verify_image_format(supported_fmts=['vmdk'])
> +
> +def blockdev_create(vm, options):
> + result = vm.qmp_log('blockdev-create', job_id='job0', options=options)
> +
> + if 'return' in result:
> + assert result['return'] == {}
> + vm.run_job('job0')
> + iotests.log("")
> +
> +with iotests.FilePath('t.vmdk') as disk_path, \
> + iotests.FilePath('t.vmdk.1') as extent1_path, \
> + iotests.FilePath('t.vmdk.2') as extent2_path, \
> + iotests.FilePath('t.vmdk.3') as extent3_path, \
> + iotests.VM() as vm:
> +
> + #
> + # Successful image creation (defaults)
> + #
> + iotests.log("=== Successful image creation (defaults) ===")
> + iotests.log("")
> +
> + size = 5 * 1024 * 1024 * 1024
> +
> + vm.launch()
> + blockdev_create(vm, { 'driver': 'file',
> + 'filename': disk_path,
> + 'size': 0 })
> +
> + vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
> + node_name='imgfile')
> +
> + blockdev_create(vm, { 'driver': imgfmt,
> + 'file': 'imgfile',
> + 'size': size })
> + vm.shutdown()
> +
> + iotests.img_info_log(disk_path)
> +
> + #
> + # Successful image creation (inline blockdev-add, explicit defaults)
> + #
> + iotests.log("=== Successful image creation (inline blockdev-add, explicit defaults) ===")
> + iotests.log("")
> +
> + # Choose a different size to show that we got a new image
> + size = 64 * 1024 * 1024
> +
> + vm.launch()
> + blockdev_create(vm, { 'driver': 'file',
> + 'filename': disk_path,
> + 'size': 0 })
> +
> + blockdev_create(vm, { 'driver': imgfmt,
> + 'file': {
> + 'driver': 'file',
> + 'filename': disk_path,
> + },
> + 'size': size,
> + 'extents': [],
> + 'subformat': 'monolithicSparse',
> + 'adapter-type': 'ide',
> + 'hwversion': '4',
> + 'zeroed-grain': False })
> + vm.shutdown()
> +
> + iotests.img_info_log(disk_path)
> +
> + #
> + # Successful image creation (non-default options)
> + #
> + iotests.log("=== Successful image creation (with non-default options) ===")
> + iotests.log("")
> +
> + # Choose a different size to show that we got a new image
> + size = 32 * 1024 * 1024
> +
> + vm.launch()
> + blockdev_create(vm, { 'driver': 'file',
> + 'filename': disk_path,
> + 'size': 0 })
> +
> + blockdev_create(vm, { 'driver': imgfmt,
> + 'file': {
> + 'driver': 'file',
> + 'filename': disk_path,
> + },
> + 'size': size,
> + 'extents': [],
> + 'subformat': 'monolithicSparse',
> + 'adapter-type': 'buslogic',
> + 'zeroed-grain': True })
> + vm.shutdown()
> +
> + iotests.img_info_log(disk_path)
> +
> + #
> + # Invalid BlockdevRef
> + #
> + iotests.log("=== Invalid BlockdevRef ===")
> + iotests.log("")
> +
> + vm.launch()
> + blockdev_create(vm, { 'driver': imgfmt,
> + 'file': "this doesn't exist",
> + 'size': size })
> + vm.shutdown()
> +
> + #
> + # Adapter types
> + #
> +
> + iotests.log("=== Adapter types ===")
> + iotests.log("")
> +
> + vm.add_blockdev('driver=file,filename=%s,node-name=node0' % (disk_path))
> +
> + # Valid
> + iotests.log("== Valid adapter types ==")
> + iotests.log("")
> +
> + vm.launch()
> + for adapter_type in [ 'ide', 'buslogic', 'lsilogic', 'legacyESX' ]:
> + blockdev_create(vm, { 'driver': imgfmt,
> + 'file': 'node0',
> + 'size': size,
> + 'adapter-type': adapter_type })
> + vm.shutdown()
> +
> + # Invalid
> + iotests.log("== Invalid adapter types ==")
> + iotests.log("")
> +
> + vm.launch()
> + for adapter_type in [ 'foo', 'IDE', 'legacyesx', 1 ]:
> + blockdev_create(vm, { 'driver': imgfmt,
> + 'file': 'node0',
> + 'size': size,
> + 'adapter-type': adapter_type })
> + vm.shutdown()
> +
> + #
> + # Other subformats
> + #
> + iotests.log("=== Other subformats ===")
> + iotests.log("")
> +
> + for path in [ extent1_path, extent2_path, extent3_path ]:
> + msg = iotests.qemu_img_pipe('create', '-f', imgfmt, path, '0')
> + iotests.log(msg, [iotests.filter_testfiles])
> +
You lost
+ vm.add_blockdev('driver=file,filename=%s,node-name=node0' % (disk_path))
here since v3. Intentional?
> + vm.add_blockdev('driver=file,filename=%s,node-name=ext1' % (extent1_path))
> + vm.add_blockdev('driver=file,filename=%s,node-name=ext2' % (extent2_path))
> + vm.add_blockdev('driver=file,filename=%s,node-name=ext3' % (extent3_path))
[...]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-create
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-create Kevin Wolf
2018-12-07 13:02 ` Markus Armbruster
@ 2018-12-07 13:12 ` Markus Armbruster
2018-12-07 14:45 ` Kevin Wolf
1 sibling, 1 reply; 19+ messages in thread
From: Markus Armbruster @ 2018-12-07 13:12 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, mreitz
git-am complains
Applying: iotests: Add VMDK tests for blockdev-create
.git/rebase-apply/patch:281: trailing whitespace.
format:
.git/rebase-apply/patch:308: trailing whitespace.
format:
.git/rebase-apply/patch:335: trailing whitespace.
format:
.git/rebase-apply/patch:600: new blank line at EOF.
+
warning: 4 lines add whitespace errors.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-create
2018-12-07 13:12 ` Markus Armbruster
@ 2018-12-07 14:45 ` Kevin Wolf
2018-12-07 15:40 ` Eric Blake
0 siblings, 1 reply; 19+ messages in thread
From: Kevin Wolf @ 2018-12-07 14:45 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-block, qemu-devel, mreitz
Am 07.12.2018 um 14:12 hat Markus Armbruster geschrieben:
> git-am complains
>
> Applying: iotests: Add VMDK tests for blockdev-create
> .git/rebase-apply/patch:281: trailing whitespace.
> format:
> .git/rebase-apply/patch:308: trailing whitespace.
> format:
> .git/rebase-apply/patch:335: trailing whitespace.
> format:
> .git/rebase-apply/patch:600: new blank line at EOF.
> +
> warning: 4 lines add whitespace errors.
This is in the reference output, so trailing whitespace/blank lines are
actually correct.
Kevin
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-create
2018-12-07 14:45 ` Kevin Wolf
@ 2018-12-07 15:40 ` Eric Blake
2018-12-07 16:42 ` Kevin Wolf
0 siblings, 1 reply; 19+ messages in thread
From: Eric Blake @ 2018-12-07 15:40 UTC (permalink / raw)
To: Kevin Wolf, Markus Armbruster; +Cc: qemu-devel, qemu-block, mreitz
On 12/7/18 8:45 AM, Kevin Wolf wrote:
> Am 07.12.2018 um 14:12 hat Markus Armbruster geschrieben:
>> git-am complains
>>
>> Applying: iotests: Add VMDK tests for blockdev-create
>> .git/rebase-apply/patch:281: trailing whitespace.
>> format:
>> .git/rebase-apply/patch:308: trailing whitespace.
>> format:
>> .git/rebase-apply/patch:335: trailing whitespace.
>> format:
>> .git/rebase-apply/patch:600: new blank line at EOF.
>> +
>> warning: 4 lines add whitespace errors.
>
> This is in the reference output, so trailing whitespace/blank lines are
> actually correct.
Ah, but doesn't ./check already ignore differences in trailing
whitespace present in the actual running that is not present in the
*.out files, precisely so we don't have to check in trailing whitespace
reference outputs?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-create
2018-12-07 15:40 ` Eric Blake
@ 2018-12-07 16:42 ` Kevin Wolf
2018-12-07 16:52 ` Eric Blake
0 siblings, 1 reply; 19+ messages in thread
From: Kevin Wolf @ 2018-12-07 16:42 UTC (permalink / raw)
To: Eric Blake; +Cc: Markus Armbruster, qemu-devel, qemu-block, mreitz
Am 07.12.2018 um 16:40 hat Eric Blake geschrieben:
> On 12/7/18 8:45 AM, Kevin Wolf wrote:
> > Am 07.12.2018 um 14:12 hat Markus Armbruster geschrieben:
> > > git-am complains
> > >
> > > Applying: iotests: Add VMDK tests for blockdev-create
> > > .git/rebase-apply/patch:281: trailing whitespace.
> > > format:
> > > .git/rebase-apply/patch:308: trailing whitespace.
> > > format:
> > > .git/rebase-apply/patch:335: trailing whitespace.
> > > format:
> > > .git/rebase-apply/patch:600: new blank line at EOF.
> > > +
> > > warning: 4 lines add whitespace errors.
> >
> > This is in the reference output, so trailing whitespace/blank lines are
> > actually correct.
>
> Ah, but doesn't ./check already ignore differences in trailing whitespace
> present in the actual running that is not present in the *.out files,
> precisely so we don't have to check in trailing whitespace reference
> outputs?
It does ignore whitespace changes, so even if we remove that whitespace,
the test won't fail. But I don't think that's a good reason to check in
inaccurate reference output.
There are a few test cases that have a reference output like this and
it's always annoying: When I later add a new subtest, I add the new test
code, review the ./check output and if it looks good, I do something
like 'cp 237.out.bad 237.out'. At that point, I'll have to manually
revert completely unrelated whitespace changes again.
Some 'git am' warnings feel like the lesser evil to me.
Kevin
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-create
2018-12-07 16:42 ` Kevin Wolf
@ 2018-12-07 16:52 ` Eric Blake
2018-12-07 17:09 ` Kevin Wolf
0 siblings, 1 reply; 19+ messages in thread
From: Eric Blake @ 2018-12-07 16:52 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Markus Armbruster, qemu-devel, qemu-block, mreitz
On 12/7/18 10:42 AM, Kevin Wolf wrote:
> Am 07.12.2018 um 16:40 hat Eric Blake geschrieben:
>> On 12/7/18 8:45 AM, Kevin Wolf wrote:
>>> Am 07.12.2018 um 14:12 hat Markus Armbruster geschrieben:
>>>> git-am complains
>>>>
>>>> Applying: iotests: Add VMDK tests for blockdev-create
>>>> .git/rebase-apply/patch:281: trailing whitespace.
>>>> format:
>>>> .git/rebase-apply/patch:308: trailing whitespace.
>>>> format:
>>>> .git/rebase-apply/patch:335: trailing whitespace.
>>>> format:
>>>> .git/rebase-apply/patch:600: new blank line at EOF.
>>>> +
>>>> warning: 4 lines add whitespace errors.
>>>
>>> This is in the reference output, so trailing whitespace/blank lines are
>>> actually correct.
>>
>> Ah, but doesn't ./check already ignore differences in trailing whitespace
>> present in the actual running that is not present in the *.out files,
>> precisely so we don't have to check in trailing whitespace reference
>> outputs?
>
> It does ignore whitespace changes, so even if we remove that whitespace,
> the test won't fail. But I don't think that's a good reason to check in
> inaccurate reference output.
>
> There are a few test cases that have a reference output like this and
> it's always annoying: When I later add a new subtest, I add the new test
> code, review the ./check output and if it looks good, I do something
> like 'cp 237.out.bad 237.out'. At that point, I'll have to manually
> revert completely unrelated whitespace changes again.
Is it worth teaching iotests.img_info_log() to strip trailing
whitespace? Or even to teach qemu-img itself to quit generating
trailing whitespace?
>
> Some 'git am' warnings feel like the lesser evil to me.
True, and a comment in the commit message about intentionally triggering
a known checkpatch flag goes a long ways to document why you want the
trailing whitespace (assuming we don't instead decide to tackle a root
cause of having the whitespace in the first place).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-create
2018-12-07 16:52 ` Eric Blake
@ 2018-12-07 17:09 ` Kevin Wolf
0 siblings, 0 replies; 19+ messages in thread
From: Kevin Wolf @ 2018-12-07 17:09 UTC (permalink / raw)
To: Eric Blake; +Cc: Markus Armbruster, qemu-devel, qemu-block, mreitz
Am 07.12.2018 um 17:52 hat Eric Blake geschrieben:
> On 12/7/18 10:42 AM, Kevin Wolf wrote:
> > Am 07.12.2018 um 16:40 hat Eric Blake geschrieben:
> > > On 12/7/18 8:45 AM, Kevin Wolf wrote:
> > > > Am 07.12.2018 um 14:12 hat Markus Armbruster geschrieben:
> > > > > git-am complains
> > > > >
> > > > > Applying: iotests: Add VMDK tests for blockdev-create
> > > > > .git/rebase-apply/patch:281: trailing whitespace.
> > > > > format:
> > > > > .git/rebase-apply/patch:308: trailing whitespace.
> > > > > format:
> > > > > .git/rebase-apply/patch:335: trailing whitespace.
> > > > > format:
> > > > > .git/rebase-apply/patch:600: new blank line at EOF.
> > > > > +
> > > > > warning: 4 lines add whitespace errors.
> > > >
> > > > This is in the reference output, so trailing whitespace/blank lines are
> > > > actually correct.
> > >
> > > Ah, but doesn't ./check already ignore differences in trailing whitespace
> > > present in the actual running that is not present in the *.out files,
> > > precisely so we don't have to check in trailing whitespace reference
> > > outputs?
> >
> > It does ignore whitespace changes, so even if we remove that whitespace,
> > the test won't fail. But I don't think that's a good reason to check in
> > inaccurate reference output.
> >
> > There are a few test cases that have a reference output like this and
> > it's always annoying: When I later add a new subtest, I add the new test
> > code, review the ./check output and if it looks good, I do something
> > like 'cp 237.out.bad 237.out'. At that point, I'll have to manually
> > revert completely unrelated whitespace changes again.
>
> Is it worth teaching iotests.img_info_log() to strip trailing whitespace?
> Or even to teach qemu-img itself to quit generating trailing whitespace?
Not sure if it's worth it, but if someone proposed such a change, I
wouldn't object anyway.
> > Some 'git am' warnings feel like the lesser evil to me.
>
> True, and a comment in the commit message about intentionally triggering a
> known checkpatch flag goes a long ways to document why you want the trailing
> whitespace (assuming we don't instead decide to tackle a root cause of
> having the whitespace in the first place).
Fair enough. Once it's merged, it doesn't make a difference any more,
but it could have made review a bit easier.
Kevin
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 5/5] vmdk: Reject excess extents in blockdev-create
2018-12-07 11:53 [Qemu-devel] [PATCH v4 0/5] vmdk: Implement blockdev-create Kevin Wolf
` (3 preceding siblings ...)
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-create Kevin Wolf
@ 2018-12-07 11:53 ` Kevin Wolf
2018-12-07 13:11 ` Markus Armbruster
2018-12-07 12:56 ` [Qemu-devel] [PATCH v4 0/5] vmdk: Implement blockdev-create no-reply
5 siblings, 1 reply; 19+ messages in thread
From: Kevin Wolf @ 2018-12-07 11:53 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, armbru, eblake, qemu-devel
Clarify that the number of extents provided in BlockdevCreateOptionsVmdk
must match the number of extents that will actually be used. Providing
more extents will result in an error now.
This requires adapting the test case to provide the right number of
extents.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 3 ++-
block/vmdk.c | 29 ++++++++++++++++++++++++-----
tests/qemu-iotests/237 | 6 +++++-
tests/qemu-iotests/237.out | 13 +++++++------
4 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0793550cf2..afdd2f89a2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4068,7 +4068,8 @@
# 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.
+# calculated as extent_number = virtual_size / 2GB. Providing
+# more extents than will be used is an error.
# @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.
diff --git a/block/vmdk.c b/block/vmdk.c
index 5a162ee85c..682ad93aa1 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1970,6 +1970,7 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
{
int extent_idx;
BlockBackend *blk = NULL;
+ BlockBackend *extent_blk;
Error *local_err = NULL;
char *desc = NULL;
int ret = 0;
@@ -2107,7 +2108,6 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
}
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);
@@ -2121,6 +2121,17 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
extent_idx++;
blk_unref(extent_blk);
}
+
+ /* Check whether we got excess extents */
+ extent_blk = extent_fn(-1, extent_idx, flat, split, compress, zeroed_grain,
+ opaque, NULL);
+ if (extent_blk) {
+ blk_unref(extent_blk);
+ error_setg(errp, "List of extents contains unused extents");
+ ret = -EINVAL;
+ goto exit;
+ }
+
/* generate descriptor file */
desc = g_strdup_printf(desc_template,
g_random_int(),
@@ -2181,6 +2192,12 @@ static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
char *ext_filename = NULL;
char *rel_filename = NULL;
+ /* We're done, don't create excess extents. */
+ if (size == -1) {
+ assert(errp == NULL);
+ return NULL;
+ }
+
if (idx == 0) {
rel_filename = g_strdup_printf("%s%s", data->prefix, data->postfix);
} else if (split) {
@@ -2342,10 +2359,12 @@ static BlockBackend *vmdk_co_create_cb(int64_t size, int idx,
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;
+ if (size != -1) {
+ ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain, errp);
+ if (ret) {
+ blk_unref(blk);
+ blk = NULL;
+ }
}
return blk;
}
diff --git a/tests/qemu-iotests/237 b/tests/qemu-iotests/237
index e04a1ac6be..251771d7fb 100755
--- a/tests/qemu-iotests/237
+++ b/tests/qemu-iotests/237
@@ -20,6 +20,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
+import math
import iotests
from iotests import imgfmt
@@ -222,12 +223,15 @@ with iotests.FilePath('t.vmdk') as disk_path, \
iotests.log("= %s %d =" % (subfmt, size))
iotests.log("")
+ num_extents = math.ceil(size / 2.0**31)
+ extents = [ "ext%d" % (i) for i in range(1, num_extents + 1) ]
+
vm.launch()
blockdev_create(vm, { 'driver': imgfmt,
'file': 'node0',
'size': size,
'subformat': subfmt,
- 'extents': ['ext1', 'ext2', 'ext3'] })
+ 'extents': extents })
vm.shutdown()
iotests.img_info_log(disk_path)
diff --git a/tests/qemu-iotests/237.out b/tests/qemu-iotests/237.out
index 1aa9aad349..241c864369 100644
--- a/tests/qemu-iotests/237.out
+++ b/tests/qemu-iotests/237.out
@@ -154,6 +154,7 @@ Job failed: Extent [0] not specified
{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 512, "subformat": "monolithicFlat"}}}
{"return": {}}
+Job failed: List of extents contains unused extents
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
@@ -161,7 +162,7 @@ Job failed: Extent [0] not specified
= twoGbMaxExtentFlat 512 =
-{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentFlat"}}}
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentFlat"}}}
{"return": {}}
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
@@ -181,7 +182,7 @@ Format specific information:
= twoGbMaxExtentSparse 512 =
-{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentSparse"}}}
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentSparse"}}}
{"return": {}}
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
@@ -203,7 +204,7 @@ Format specific information:
= twoGbMaxExtentFlat 1073741824 =
-{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentFlat"}}}
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentFlat"}}}
{"return": {}}
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
@@ -223,7 +224,7 @@ Format specific information:
= twoGbMaxExtentSparse 1073741824 =
-{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentSparse"}}}
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentSparse"}}}
{"return": {}}
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
@@ -245,7 +246,7 @@ Format specific information:
= twoGbMaxExtentFlat 2147483648 =
-{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 2147483648, "subformat": "twoGbMaxExtentFlat"}}}
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 2147483648, "subformat": "twoGbMaxExtentFlat"}}}
{"return": {}}
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
@@ -265,7 +266,7 @@ Format specific information:
= twoGbMaxExtentSparse 2147483648 =
-{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 2147483648, "subformat": "twoGbMaxExtentSparse"}}}
+{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 2147483648, "subformat": "twoGbMaxExtentSparse"}}}
{"return": {}}
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
--
2.19.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v4 5/5] vmdk: Reject excess extents in blockdev-create
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 5/5] vmdk: Reject excess extents in blockdev-create Kevin Wolf
@ 2018-12-07 13:11 ` Markus Armbruster
2018-12-07 14:54 ` Kevin Wolf
0 siblings, 1 reply; 19+ messages in thread
From: Markus Armbruster @ 2018-12-07 13:11 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, mreitz
Kevin Wolf <kwolf@redhat.com> writes:
> Clarify that the number of extents provided in BlockdevCreateOptionsVmdk
> must match the number of extents that will actually be used. Providing
> more extents will result in an error now.
>
> This requires adapting the test case to provide the right number of
> extents.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 3 ++-
> block/vmdk.c | 29 ++++++++++++++++++++++++-----
> tests/qemu-iotests/237 | 6 +++++-
> tests/qemu-iotests/237.out | 13 +++++++------
> 4 files changed, 38 insertions(+), 13 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0793550cf2..afdd2f89a2 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4068,7 +4068,8 @@
> # 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.
> +# calculated as extent_number = virtual_size / 2GB. Providing
> +# more extents than will be used is an error.
> # @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.
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 5a162ee85c..682ad93aa1 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1970,6 +1970,7 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
> {
> int extent_idx;
> BlockBackend *blk = NULL;
> + BlockBackend *extent_blk;
> Error *local_err = NULL;
> char *desc = NULL;
> int ret = 0;
> @@ -2107,7 +2108,6 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
> }
> 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);
> @@ -2121,6 +2121,17 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
> extent_idx++;
> blk_unref(extent_blk);
> }
> +
> + /* Check whether we got excess extents */
> + extent_blk = extent_fn(-1, extent_idx, flat, split, compress, zeroed_grain,
> + opaque, NULL);
> + if (extent_blk) {
> + blk_unref(extent_blk);
> + error_setg(errp, "List of extents contains unused extents");
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> /* generate descriptor file */
> desc = g_strdup_printf(desc_template,
> g_random_int(),
> @@ -2181,6 +2192,12 @@ static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
> char *ext_filename = NULL;
> char *rel_filename = NULL;
>
> + /* We're done, don't create excess extents. */
> + if (size == -1) {
> + assert(errp == NULL);
> + return NULL;
> + }
> +
I'm afraid I don't get this hunk.
> if (idx == 0) {
> rel_filename = g_strdup_printf("%s%s", data->prefix, data->postfix);
> } else if (split) {
> @@ -2342,10 +2359,12 @@ static BlockBackend *vmdk_co_create_cb(int64_t size, int idx,
> 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;
> + if (size != -1) {
> + ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain, errp);
> + if (ret) {
> + blk_unref(blk);
> + blk = NULL;
> + }
> }
> return blk;
> }
> diff --git a/tests/qemu-iotests/237 b/tests/qemu-iotests/237
> index e04a1ac6be..251771d7fb 100755
> --- a/tests/qemu-iotests/237
> +++ b/tests/qemu-iotests/237
> @@ -20,6 +20,7 @@
> # along with this program. If not, see <http://www.gnu.org/licenses/>.
> #
>
> +import math
> import iotests
> from iotests import imgfmt
>
> @@ -222,12 +223,15 @@ with iotests.FilePath('t.vmdk') as disk_path, \
> iotests.log("= %s %d =" % (subfmt, size))
> iotests.log("")
>
> + num_extents = math.ceil(size / 2.0**31)
> + extents = [ "ext%d" % (i) for i in range(1, num_extents + 1) ]
> +
> vm.launch()
> blockdev_create(vm, { 'driver': imgfmt,
> 'file': 'node0',
> 'size': size,
> 'subformat': subfmt,
> - 'extents': ['ext1', 'ext2', 'ext3'] })
> + 'extents': extents })
> vm.shutdown()
>
> iotests.img_info_log(disk_path)
> diff --git a/tests/qemu-iotests/237.out b/tests/qemu-iotests/237.out
> index 1aa9aad349..241c864369 100644
> --- a/tests/qemu-iotests/237.out
> +++ b/tests/qemu-iotests/237.out
> @@ -154,6 +154,7 @@ Job failed: Extent [0] not specified
>
> {"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 512, "subformat": "monolithicFlat"}}}
> {"return": {}}
> +Job failed: List of extents contains unused extents
> {"execute": "job-dismiss", "arguments": {"id": "job0"}}
> {"return": {}}
>
> @@ -161,7 +162,7 @@ Job failed: Extent [0] not specified
>
> = twoGbMaxExtentFlat 512 =
>
> -{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentFlat"}}}
> +{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentFlat"}}}
> {"return": {}}
> {"execute": "job-dismiss", "arguments": {"id": "job0"}}
> {"return": {}}
> @@ -181,7 +182,7 @@ Format specific information:
>
> = twoGbMaxExtentSparse 512 =
>
> -{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentSparse"}}}
> +{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 512, "subformat": "twoGbMaxExtentSparse"}}}
> {"return": {}}
> {"execute": "job-dismiss", "arguments": {"id": "job0"}}
> {"return": {}}
> @@ -203,7 +204,7 @@ Format specific information:
>
> = twoGbMaxExtentFlat 1073741824 =
>
> -{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentFlat"}}}
> +{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentFlat"}}}
> {"return": {}}
> {"execute": "job-dismiss", "arguments": {"id": "job0"}}
> {"return": {}}
> @@ -223,7 +224,7 @@ Format specific information:
>
> = twoGbMaxExtentSparse 1073741824 =
>
> -{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentSparse"}}}
> +{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 1073741824, "subformat": "twoGbMaxExtentSparse"}}}
> {"return": {}}
> {"execute": "job-dismiss", "arguments": {"id": "job0"}}
> {"return": {}}
> @@ -245,7 +246,7 @@ Format specific information:
>
> = twoGbMaxExtentFlat 2147483648 =
>
> -{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 2147483648, "subformat": "twoGbMaxExtentFlat"}}}
> +{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 2147483648, "subformat": "twoGbMaxExtentFlat"}}}
> {"return": {}}
> {"execute": "job-dismiss", "arguments": {"id": "job0"}}
> {"return": {}}
> @@ -265,7 +266,7 @@ Format specific information:
>
> = twoGbMaxExtentSparse 2147483648 =
>
> -{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", "size": 2147483648, "subformat": "twoGbMaxExtentSparse"}}}
> +{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 2147483648, "subformat": "twoGbMaxExtentSparse"}}}
> {"return": {}}
> {"execute": "job-dismiss", "arguments": {"id": "job0"}}
> {"return": {}}
Apart from the hunk that stumps me, the patch looks good to me.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v4 5/5] vmdk: Reject excess extents in blockdev-create
2018-12-07 13:11 ` Markus Armbruster
@ 2018-12-07 14:54 ` Kevin Wolf
2018-12-07 15:34 ` Markus Armbruster
0 siblings, 1 reply; 19+ messages in thread
From: Kevin Wolf @ 2018-12-07 14:54 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-block, qemu-devel, mreitz
Am 07.12.2018 um 14:11 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Clarify that the number of extents provided in BlockdevCreateOptionsVmdk
> > must match the number of extents that will actually be used. Providing
> > more extents will result in an error now.
> >
> > This requires adapting the test case to provide the right number of
> > extents.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > qapi/block-core.json | 3 ++-
> > block/vmdk.c | 29 ++++++++++++++++++++++++-----
> > tests/qemu-iotests/237 | 6 +++++-
> > tests/qemu-iotests/237.out | 13 +++++++------
> > 4 files changed, 38 insertions(+), 13 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 0793550cf2..afdd2f89a2 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4068,7 +4068,8 @@
> > # 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.
> > +# calculated as extent_number = virtual_size / 2GB. Providing
> > +# more extents than will be used is an error.
> > # @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.
> > diff --git a/block/vmdk.c b/block/vmdk.c
> > index 5a162ee85c..682ad93aa1 100644
> > --- a/block/vmdk.c
> > +++ b/block/vmdk.c
> > @@ -1970,6 +1970,7 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
> > {
> > int extent_idx;
> > BlockBackend *blk = NULL;
> > + BlockBackend *extent_blk;
> > Error *local_err = NULL;
> > char *desc = NULL;
> > int ret = 0;
> > @@ -2107,7 +2108,6 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
> > }
> > 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);
> > @@ -2121,6 +2121,17 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
> > extent_idx++;
> > blk_unref(extent_blk);
> > }
> > +
> > + /* Check whether we got excess extents */
> > + extent_blk = extent_fn(-1, extent_idx, flat, split, compress, zeroed_grain,
> > + opaque, NULL);
> > + if (extent_blk) {
> > + blk_unref(extent_blk);
> > + error_setg(errp, "List of extents contains unused extents");
> > + ret = -EINVAL;
> > + goto exit;
> > + }
> > +
> > /* generate descriptor file */
> > desc = g_strdup_printf(desc_template,
> > g_random_int(),
> > @@ -2181,6 +2192,12 @@ static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
> > char *ext_filename = NULL;
> > char *rel_filename = NULL;
> >
> > + /* We're done, don't create excess extents. */
> > + if (size == -1) {
> > + assert(errp == NULL);
> > + return NULL;
> > + }
> > +
>
> I'm afraid I don't get this hunk.
vmdk_co_create_opts_cb() is for the legacy code path, where 'qemu-img
create' passes us a QemuOpts. This doesn't contain extents, so rather
than returning ther next extent from the input, this function simply
creates a new extent file each time it is called.
When checking whether we have too many extents, we don't know from which
code path we came, so we have to call the callback uncondtionally and
see whether it still returns an extra extent. In this case, creating a
new one would obviously be counterproductive and trigger an error, so I
just return NULL immediately.
If you have a suggestion for a comment to put somewhere, I'll gladly add
it.
Kevin
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v4 5/5] vmdk: Reject excess extents in blockdev-create
2018-12-07 14:54 ` Kevin Wolf
@ 2018-12-07 15:34 ` Markus Armbruster
0 siblings, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2018-12-07 15:34 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz
Kevin Wolf <kwolf@redhat.com> writes:
> Am 07.12.2018 um 14:11 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Clarify that the number of extents provided in BlockdevCreateOptionsVmdk
>> > must match the number of extents that will actually be used. Providing
>> > more extents will result in an error now.
>> >
>> > This requires adapting the test case to provide the right number of
>> > extents.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> > qapi/block-core.json | 3 ++-
>> > block/vmdk.c | 29 ++++++++++++++++++++++++-----
>> > tests/qemu-iotests/237 | 6 +++++-
>> > tests/qemu-iotests/237.out | 13 +++++++------
>> > 4 files changed, 38 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index 0793550cf2..afdd2f89a2 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -4068,7 +4068,8 @@
>> > # 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.
>> > +# calculated as extent_number = virtual_size / 2GB. Providing
>> > +# more extents than will be used is an error.
>> > # @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.
>> > diff --git a/block/vmdk.c b/block/vmdk.c
>> > index 5a162ee85c..682ad93aa1 100644
>> > --- a/block/vmdk.c
>> > +++ b/block/vmdk.c
>> > @@ -1970,6 +1970,7 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
>> > {
>> > int extent_idx;
>> > BlockBackend *blk = NULL;
>> > + BlockBackend *extent_blk;
>> > Error *local_err = NULL;
>> > char *desc = NULL;
>> > int ret = 0;
>> > @@ -2107,7 +2108,6 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
>> > }
>> > 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);
>> > @@ -2121,6 +2121,17 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
>> > extent_idx++;
>> > blk_unref(extent_blk);
>> > }
>> > +
>> > + /* Check whether we got excess extents */
>> > + extent_blk = extent_fn(-1, extent_idx, flat, split, compress, zeroed_grain,
>> > + opaque, NULL);
>> > + if (extent_blk) {
>> > + blk_unref(extent_blk);
>> > + error_setg(errp, "List of extents contains unused extents");
>> > + ret = -EINVAL;
>> > + goto exit;
>> > + }
>> > +
>> > /* generate descriptor file */
>> > desc = g_strdup_printf(desc_template,
>> > g_random_int(),
>> > @@ -2181,6 +2192,12 @@ static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
>> > char *ext_filename = NULL;
>> > char *rel_filename = NULL;
>> >
>> > + /* We're done, don't create excess extents. */
>> > + if (size == -1) {
>> > + assert(errp == NULL);
>> > + return NULL;
>> > + }
>> > +
>>
>> I'm afraid I don't get this hunk.
>
> vmdk_co_create_opts_cb() is for the legacy code path, where 'qemu-img
> create' passes us a QemuOpts. This doesn't contain extents, so rather
> than returning ther next extent from the input, this function simply
> creates a new extent file each time it is called.
>
> When checking whether we have too many extents, we don't know from which
> code path we came, so we have to call the callback uncondtionally and
> see whether it still returns an extra extent. In this case, creating a
> new one would obviously be counterproductive and trigger an error, so I
> just return NULL immediately.
>
> If you have a suggestion for a comment to put somewhere, I'll gladly add
> it.
Coming up with intelligble comments exceed my mental capabilities right
now, I'm afraid. But I can still give my
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/5] vmdk: Implement blockdev-create
2018-12-07 11:53 [Qemu-devel] [PATCH v4 0/5] vmdk: Implement blockdev-create Kevin Wolf
` (4 preceding siblings ...)
2018-12-07 11:53 ` [Qemu-devel] [PATCH v4 5/5] vmdk: Reject excess extents in blockdev-create Kevin Wolf
@ 2018-12-07 12:56 ` no-reply
5 siblings, 0 replies; 19+ messages in thread
From: no-reply @ 2018-12-07 12:56 UTC (permalink / raw)
To: kwolf; +Cc: famz, qemu-block, qemu-devel, armbru, mreitz
Patchew URL: https://patchew.org/QEMU/20181207115343.6747-1-kwolf@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20181207115343.6747-1-kwolf@redhat.com
Subject: [Qemu-devel] [PATCH v4 0/5] vmdk: Implement blockdev-create
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
914b679 vmdk: Reject excess extents in blockdev-create
2721cff iotests: Add VMDK tests for blockdev-create
c098e37 iotests: Filter cid numbers in VMDK extent info
3e8f068 vmdk: Implement .bdrv_co_create callback
54b29ff vmdk: Refactor vmdk_create_extent
=== OUTPUT BEGIN ===
Checking PATCH 1/5: vmdk: Refactor vmdk_create_extent...
WARNING: line over 80 characters
#125: FILE: block/vmdk.c:2114:
+ flat, compress, zeroed_grain, NULL, opts, errp)) {
total: 0 errors, 1 warnings, 104 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/5: vmdk: Implement .bdrv_co_create callback...
WARNING: line over 80 characters
#216: FILE: block/vmdk.c:2075:
+ bdrv_get_full_backing_filename_from_filename(blk_bs(blk)->filename, backing_file,
WARNING: line over 80 characters
#371: FILE: block/vmdk.c:2174:
+ bool flat, bool split, bool compress,
WARNING: line over 80 characters
#389: FILE: block/vmdk.c:2192:
+ rel_filename = g_strdup_printf("%s-flat%s", data->prefix, data->postfix);
WARNING: line over 80 characters
#406: FILE: block/vmdk.c:2209:
+static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
total: 0 errors, 4 warnings, 658 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 3/5: iotests: Filter cid numbers in VMDK extent info...
Checking PATCH 4/5: iotests: Add VMDK tests for blockdev-create...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#11:
new file mode 100755
ERROR: trailing whitespace
#282: FILE: tests/qemu-iotests/237.out:28:
+ format: $
ERROR: trailing whitespace
#309: FILE: tests/qemu-iotests/237.out:55:
+ format: $
ERROR: trailing whitespace
#336: FILE: tests/qemu-iotests/237.out:82:
+ format: $
total: 3 errors, 1 warnings, 584 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 5/5: vmdk: Reject excess extents in blockdev-create...
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20181207115343.6747-1-kwolf@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 19+ messages in thread