* [Qemu-devel] [PATCH v3 0/4] vmdk: Implement blockdev-create
@ 2018-12-06 15:13 Kevin Wolf
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 1/4] vmdk: Refactor vmdk_create_extent Kevin Wolf
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Kevin Wolf @ 2018-12-06 15:13 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, armbru, eblake, qemu-devel
I picked up the patch series from Fam and rebased it to current master
(which involved a major rework on the test case) and tried to address
Markus' review comments for v2. I did not do any further review of the
actual code, but it passes the tests, so I guess having it in tree is
better than continuing to let it bitrot.
Fam Zheng (3):
vmdk: Refactor vmdk_create_extent
vmdk: Implement .bdrv_co_create callback
iotests: Filter cid numbers in VMDK extent info
Kevin Wolf (1):
iotests: Add VMDK tests for blockdev-create
qapi/block-core.json | 70 +++++
qapi/qapi-schema.json | 1 +
block/vmdk.c | 519 ++++++++++++++++++++++---------
tests/qemu-iotests/237 | 201 ++++++++++++
tests/qemu-iotests/237.out | 309 ++++++++++++++++++
tests/qemu-iotests/common.filter | 1 +
tests/qemu-iotests/group | 1 +
tests/qemu-iotests/iotests.py | 1 +
8 files changed, 951 insertions(+), 152 deletions(-)
create mode 100755 tests/qemu-iotests/237
create mode 100644 tests/qemu-iotests/237.out
--
2.19.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v3 1/4] vmdk: Refactor vmdk_create_extent
2018-12-06 15:13 [Qemu-devel] [PATCH v3 0/4] vmdk: Implement blockdev-create Kevin Wolf
@ 2018-12-06 15:13 ` Kevin Wolf
2018-12-07 6:40 ` Markus Armbruster
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 2/4] vmdk: Implement .bdrv_co_create callback Kevin Wolf
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Kevin Wolf @ 2018-12-06 15:13 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>
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] 12+ messages in thread
* [Qemu-devel] [PATCH v3 2/4] vmdk: Implement .bdrv_co_create callback
2018-12-06 15:13 [Qemu-devel] [PATCH v3 0/4] vmdk: Implement blockdev-create Kevin Wolf
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 1/4] vmdk: Refactor vmdk_create_extent Kevin Wolf
@ 2018-12-06 15:13 ` Kevin Wolf
2018-12-07 7:10 ` Markus Armbruster
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 3/4] iotests: Filter cid numbers in VMDK extent info Kevin Wolf
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Kevin Wolf @ 2018-12-06 15:13 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 | 1 +
block/vmdk.c | 452 ++++++++++++++++++++++++++++++------------
3 files changed, 396 insertions(+), 127 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d4fe710836..4778f88dd8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4021,6 +4021,75 @@
'size': 'size',
'*cluster-size' : 'size' } }
+##
+# @BlockdevVmdkSubformat:
+#
+# Subformat options for VMDK images
+#
+# @monolithicSparse: Single file image with sparse cluster allocation
+#
+# @monolithicFlat: Single flat data image and a descriptor file
+#
+# @twoGbMaxExtentSparse: Data is split into 2GB (per virtual LBA) sparse extent
+# files, in addition to a descriptor file
+#
+# @twoGbMaxExtentFlat: Data is split into 2GB (per virtual LBA) flat extent
+# files, in addition to a descriptor file
+#
+# @streamOptimized: Single file image sparse cluster allocation, optimized
+# for streaming over network.
+#
+# Since: 4.0
+##
+{ 'enum': 'BlockdevVmdkSubformat',
+ 'data': [ 'monolithicSparse', 'monolithicFlat', 'twoGbMaxExtentSparse',
+ 'twoGbMaxExtentFlat', 'streamOptimized'] }
+
+##
+# @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".
+# Defaulted to "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..78e8bcd561 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -66,6 +66,7 @@
'ACPISlotType', # DIMM, visible through query-acpi-ospm-status
'CpuInfoMIPS', # PC, visible through query-cpu
'CpuInfoTricore', # PC, visible through query-cpu
+ 'BlockdevVmdkSubformat',# all members, to match VMDK spec spellings
'QapiErrorClass', # all members, visible through errors
'UuidInfo', # UUID, visible through query-uuid
'X86CPURegister32', # all members, visible indirectly through qom-get
diff --git a/block/vmdk.c b/block/vmdk.c
index 32fc2c84b3..16f86457d7 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1932,33 +1932,56 @@ static int filename_decompose(const char *filename, char *path, char *prefix,
return VMDK_OK;
}
-static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
- Error **errp)
+/*
+ * idx == 0: get or create the descriptor file (also the image file if in a
+ * non-split format.
+ * idx >= 1: get the n-th extent if in a split subformat
+ */
+typedef BlockBackend *(*vmdk_create_extent_fn)(int64_t size,
+ int idx,
+ bool flat,
+ bool split,
+ bool compress,
+ bool zeroed_grain,
+ void *opaque,
+ Error **errp);
+
+static void vmdk_desc_add_extent(GString *desc,
+ const char *extent_line_fmt,
+ int64_t size, const char *filename)
+{
+ char *basename = g_path_get_basename(filename);
+ 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,215 @@ 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;
+ int i;
+ 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) {
+ for (i = 0; i < strlen(adapter_type); ++i) {
+ adapter_type[i] = qemu_tolower(adapter_type[i]);
+ }
+ 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 +2303,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 +2648,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] 12+ messages in thread
* [Qemu-devel] [PATCH v3 3/4] iotests: Filter cid numbers in VMDK extent info
2018-12-06 15:13 [Qemu-devel] [PATCH v3 0/4] vmdk: Implement blockdev-create Kevin Wolf
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 1/4] vmdk: Refactor vmdk_create_extent Kevin Wolf
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 2/4] vmdk: Implement .bdrv_co_create callback Kevin Wolf
@ 2018-12-06 15:13 ` Kevin Wolf
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 4/4] iotests: Add VMDK tests for blockdev-create Kevin Wolf
2018-12-06 20:10 ` [Qemu-devel] [PATCH v3 0/4] vmdk: Implement blockdev-create no-reply
4 siblings, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2018-12-06 15:13 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] 12+ messages in thread
* [Qemu-devel] [PATCH v3 4/4] iotests: Add VMDK tests for blockdev-create
2018-12-06 15:13 [Qemu-devel] [PATCH v3 0/4] vmdk: Implement blockdev-create Kevin Wolf
` (2 preceding siblings ...)
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 3/4] iotests: Filter cid numbers in VMDK extent info Kevin Wolf
@ 2018-12-06 15:13 ` Kevin Wolf
2018-12-06 20:10 ` [Qemu-devel] [PATCH v3 0/4] vmdk: Implement blockdev-create no-reply
4 siblings, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2018-12-06 15:13 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 | 201 ++++++++++++++++++++++++
tests/qemu-iotests/237.out | 309 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 511 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..08e575bea2
--- /dev/null
+++ b/tests/qemu-iotests/237
@@ -0,0 +1,201 @@
+#!/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()
+
+ #
+ # 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=node0' % (disk_path))
+ 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..d6557826be
--- /dev/null
+++ b/tests/qemu-iotests/237.out
@@ -0,0 +1,309 @@
+=== 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": {}}
+
+=== 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] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] vmdk: Implement blockdev-create
2018-12-06 15:13 [Qemu-devel] [PATCH v3 0/4] vmdk: Implement blockdev-create Kevin Wolf
` (3 preceding siblings ...)
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 4/4] iotests: Add VMDK tests for blockdev-create Kevin Wolf
@ 2018-12-06 20:10 ` no-reply
4 siblings, 0 replies; 12+ messages in thread
From: no-reply @ 2018-12-06 20:10 UTC (permalink / raw)
To: kwolf; +Cc: famz, qemu-block, qemu-devel, armbru, mreitz
Patchew URL: https://patchew.org/QEMU/20181206151304.8388-1-kwolf@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Subject: [Qemu-devel] [PATCH v3 0/4] vmdk: Implement blockdev-create
Message-id: 20181206151304.8388-1-kwolf@redhat.com
=== 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'
72ea84d iotests: Add VMDK tests for blockdev-create
e2fa3cf iotests: Filter cid numbers in VMDK extent info
fb76434 vmdk: Implement .bdrv_co_create callback
d16851a vmdk: Refactor vmdk_create_extent
=== OUTPUT BEGIN ===
Checking PATCH 1/4: vmdk: Refactor vmdk_create_extent...
WARNING: line over 80 characters
#124: 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/4: 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, 647 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/4: iotests: Filter cid numbers in VMDK extent info...
Checking PATCH 4/4: 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
#250: FILE: tests/qemu-iotests/237.out:28:
+ format: $
ERROR: trailing whitespace
#277: FILE: tests/qemu-iotests/237.out:55:
+ format: $
ERROR: trailing whitespace
#304: FILE: tests/qemu-iotests/237.out:82:
+ format: $
total: 3 errors, 1 warnings, 514 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.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20181206151304.8388-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] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] vmdk: Refactor vmdk_create_extent
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 1/4] vmdk: Refactor vmdk_create_extent Kevin Wolf
@ 2018-12-07 6:40 ` Markus Armbruster
2018-12-07 10:24 ` Kevin Wolf
0 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2018-12-07 6:40 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, mreitz
Kevin Wolf <kwolf@redhat.com> writes:
> 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.
I'd do this part in the next patch, when it has a user. Matter of
taste.
>
> Signed-off-by: Fam Zheng <famz@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;
Dead assignment. Matter of taste.
> + }
> }
> - 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;
> }
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] vmdk: Implement .bdrv_co_create callback
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 2/4] vmdk: Implement .bdrv_co_create callback Kevin Wolf
@ 2018-12-07 7:10 ` Markus Armbruster
2018-12-07 10:53 ` Kevin Wolf
0 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2018-12-07 7:10 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, mreitz
This is a reasonably careful review of the QAPI-related parts, but more
of an eye-over for the remainder.
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 | 1 +
> block/vmdk.c | 452 ++++++++++++++++++++++++++++++------------
> 3 files changed, 396 insertions(+), 127 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d4fe710836..4778f88dd8 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4021,6 +4021,75 @@
> 'size': 'size',
> '*cluster-size' : 'size' } }
>
> +##
> +# @BlockdevVmdkSubformat:
> +#
> +# Subformat options for VMDK images
> +#
> +# @monolithicSparse: Single file image with sparse cluster allocation
> +#
> +# @monolithicFlat: Single flat data image and a descriptor file
> +#
> +# @twoGbMaxExtentSparse: Data is split into 2GB (per virtual LBA) sparse extent
> +# files, in addition to a descriptor file
> +#
> +# @twoGbMaxExtentFlat: Data is split into 2GB (per virtual LBA) flat extent
> +# files, in addition to a descriptor file
> +#
> +# @streamOptimized: Single file image sparse cluster allocation, optimized
> +# for streaming over network.
> +#
> +# Since: 4.0
> +##
> +{ 'enum': 'BlockdevVmdkSubformat',
> + 'data': [ 'monolithicSparse', 'monolithicFlat', 'twoGbMaxExtentSparse',
> + 'twoGbMaxExtentFlat', 'streamOptimized'] }
Don't conform to the QAPI rules on names "to match VMDK spec spellings"
(see qapi-schema.json below). We discussed this in review of v1.
PRO: matches the VMDK spec (whatever that's worth), keeps the code
simple.
CON: the non-conforming names become part of the stable QMP interface,
in the argument of command blockdev-create.
I don't like the CON, but I'm willing to tolerate it in the name of
simplicity.
> +
> +##
> +# @BlockdevVmdkAdapterType:
> +#
> +# Adapter type info for VMDK images
> +#
> +# Since: 4.0
> +##
> +{ 'enum': 'BlockdevVmdkAdapterType',
> + 'data': [ 'ide', 'buslogic', 'lsilogic', 'legacyesx'] }
May I have hyphens in the composite nouns? Hmm, might be the way they
are to match VMDK spec spellings or for backward compatibility. I guess
we'll see below.
> +
> +##
> +# @BlockdevCreateOptionsVmdk:
> +#
> +# Driver specific image creation options for VMDK.
> +#
> +# @file Where to store the new image file. This refers to the image
> +# file for monolithcSparse and streamOptimized format, or the
> +# descriptor file for other formats.
> +# @size Size of the virtual disk in bytes
> +# @extents Where to store the data extents. Required for monolithcflat,
> +# twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For
> +# monolithicflat, only one entry is required; for
monolithicFlat
> +# twoGbMaxExtent* formats, the number of entries required is
> +# calculated as extent_number = virtual_size / 2GB.
Is it okay to supply more entries than required, or do I have to supply
exactly the right number?
> +# @subformat The subformat of the VMDK image. Default: "monolithicsparse".
monolithicSparse
> +# @backing-file The path of backing file. Default: no backing file is used.
> +# @adapter-type The adapter type used to fill in the descriptor. Default: ide.
> +# @hwversion Hardware version. The meaningful options are "4" or "6".
> +# Defaulted to "4".
Default: "4"
> +# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats.
> +# Default: false.
> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'BlockdevCreateOptionsVmdk',
> + 'data': { 'file': 'BlockdevRef',
> + 'size': 'size',
> + '*extents': ['BlockdevRef'],
> + '*subformat': 'BlockdevVmdkSubformat',
> + '*backing-file': 'str',
> + '*adapter-type': 'BlockdevVmdkAdapterType',
> + '*hwversion': 'str',
> + '*zeroed-grain': 'bool' } }
@zeroed-grain is undocumented.
> +
> +
> ##
> # @SheepdogRedundancyType:
> #
> @@ -4215,6 +4284,7 @@
> 'ssh': 'BlockdevCreateOptionsSsh',
> 'vdi': 'BlockdevCreateOptionsVdi',
> 'vhdx': 'BlockdevCreateOptionsVhdx',
> + 'vmdk': 'BlockdevCreateOptionsVmdk',
> 'vpc': 'BlockdevCreateOptionsVpc'
> } }
>
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index 65b6dc2f6f..78e8bcd561 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -66,6 +66,7 @@
> 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status
> 'CpuInfoMIPS', # PC, visible through query-cpu
> 'CpuInfoTricore', # PC, visible through query-cpu
> + 'BlockdevVmdkSubformat',# all members, to match VMDK spec spellings
> 'QapiErrorClass', # all members, visible through errors
> 'UuidInfo', # UUID, visible through query-uuid
> 'X86CPURegister32', # all members, visible indirectly through qom-get
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 32fc2c84b3..16f86457d7 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1932,33 +1932,56 @@ static int filename_decompose(const char *filename, char *path, char *prefix,
> return VMDK_OK;
> }
>
> -static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
> - Error **errp)
> +/*
> + * idx == 0: get or create the descriptor file (also the image file if in a
> + * non-split format.
> + * idx >= 1: get the n-th extent if in a split subformat
> + */
> +typedef BlockBackend *(*vmdk_create_extent_fn)(int64_t size,
> + int idx,
> + bool flat,
> + bool split,
> + bool compress,
> + bool zeroed_grain,
> + void *opaque,
> + Error **errp);
> +
> +static void vmdk_desc_add_extent(GString *desc,
> + const char *extent_line_fmt,
> + int64_t size, const char *filename)
> +{
> + char *basename = g_path_get_basename(filename);
I like a blank line between declarations and statements.
> + g_string_append_printf(desc, extent_line_fmt,
> + DIV_ROUND_UP(size, BDRV_SECTOR_SIZE), basename);
> +
> + g_free(basename);
> +}
> +
> +static int coroutine_fn vmdk_co_do_create(int64_t size,
> + BlockdevVmdkSubformat subformat,
> + BlockdevVmdkAdapterType adapter_type,
> + const char *backing_file,
> + const char *hw_version,
> + bool compat6,
> + bool zeroed_grain,
> + vmdk_create_extent_fn extent_fn,
> + void *opaque,
> + Error **errp)
> {
> - int idx = 0;
> - BlockBackend *new_blk = NULL;
> + int extent_idx;
> + BlockBackend *blk = NULL;
> Error *local_err = NULL;
> char *desc = NULL;
> - int64_t total_size = 0, filesize;
> - char *adapter_type = NULL;
> - char *backing_file = NULL;
> - char *hw_version = NULL;
> - char *fmt = NULL;
> int ret = 0;
> bool flat, split, compress;
> GString *ext_desc_lines;
> - char *path = g_malloc0(PATH_MAX);
> - char *prefix = g_malloc0(PATH_MAX);
> - char *postfix = g_malloc0(PATH_MAX);
> - char *desc_line = g_malloc0(BUF_SIZE);
> - char *ext_filename = g_malloc0(PATH_MAX);
> - char *desc_filename = g_malloc0(PATH_MAX);
> const int64_t split_size = 0x80000000; /* VMDK has constant split size */
> - const char *desc_extent_line;
> + int64_t extent_size;
> + int64_t created_size = 0;
> + const char *extent_line_fmt;
> char *parent_desc_line = g_malloc0(BUF_SIZE);
> uint32_t parent_cid = 0xffffffff;
> uint32_t number_heads = 16;
> - bool zeroed_grain = false;
> uint32_t desc_offset = 0, desc_len;
> const char desc_template[] =
> "# Disk DescriptorFile\n"
> @@ -1982,71 +2005,35 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
>
> ext_desc_lines = g_string_new(NULL);
>
> - if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
> - ret = -EINVAL;
> - goto exit;
> - }
> /* Read out options */
> - total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> - BDRV_SECTOR_SIZE);
> - adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
> - backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> - hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION);
> - if (qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false)) {
> - if (strcmp(hw_version, "undefined")) {
> + if (compat6) {
> + if (hw_version) {
> error_setg(errp,
> "compat6 cannot be enabled with hwversion set");
> ret = -EINVAL;
> goto exit;
> }
> - g_free(hw_version);
> - hw_version = g_strdup("6");
> - }
> - if (strcmp(hw_version, "undefined") == 0) {
> - g_free(hw_version);
> - hw_version = g_strdup("4");
> + hw_version = "6";
> }
> - fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
> - if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false)) {
> - zeroed_grain = true;
> + if (!hw_version) {
> + hw_version = "4";
> }
>
> - if (!adapter_type) {
> - adapter_type = g_strdup("ide");
> - } else if (strcmp(adapter_type, "ide") &&
> - strcmp(adapter_type, "buslogic") &&
> - strcmp(adapter_type, "lsilogic") &&
> - strcmp(adapter_type, "legacyESX")) {
> - error_setg(errp, "Unknown adapter type: '%s'", adapter_type);
> - ret = -EINVAL;
> - goto exit;
> - }
Old code recognizes "legacyESX". New code recognizes "legacyesx". Bug
or feature?
> - if (strcmp(adapter_type, "ide") != 0) {
> + if (adapter_type != BLOCKDEV_VMDK_ADAPTER_TYPE_IDE) {
> /* that's the number of heads with which vmware operates when
> creating, exporting, etc. vmdk files with a non-ide adapter type */
> number_heads = 255;
> }
> - if (!fmt) {
> - /* Default format to monolithicSparse */
> - fmt = g_strdup("monolithicSparse");
> - } else if (strcmp(fmt, "monolithicFlat") &&
> - strcmp(fmt, "monolithicSparse") &&
> - strcmp(fmt, "twoGbMaxExtentSparse") &&
> - strcmp(fmt, "twoGbMaxExtentFlat") &&
> - strcmp(fmt, "streamOptimized")) {
> - error_setg(errp, "Unknown subformat: '%s'", fmt);
> - ret = -EINVAL;
> - goto exit;
> - }
> - split = !(strcmp(fmt, "twoGbMaxExtentFlat") &&
> - strcmp(fmt, "twoGbMaxExtentSparse"));
> - flat = !(strcmp(fmt, "monolithicFlat") &&
> - strcmp(fmt, "twoGbMaxExtentFlat"));
> - compress = !strcmp(fmt, "streamOptimized");
> + split = (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT) ||
> + (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTSPARSE);
> + flat = (subformat == BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICFLAT) ||
> + (subformat == BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT);
> + compress = subformat == BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED;
> +
> if (flat) {
> - desc_extent_line = "RW %" PRId64 " FLAT \"%s\" 0\n";
> + extent_line_fmt = "RW %" PRId64 " FLAT \"%s\" 0\n";
> } else {
> - desc_extent_line = "RW %" PRId64 " SPARSE \"%s\"\n";
> + extent_line_fmt = "RW %" PRId64 " SPARSE \"%s\"\n";
> }
> if (flat && backing_file) {
> error_setg(errp, "Flat image can't have backing file");
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] vmdk: Refactor vmdk_create_extent
2018-12-07 6:40 ` Markus Armbruster
@ 2018-12-07 10:24 ` Kevin Wolf
2018-12-07 12:52 ` Markus Armbruster
0 siblings, 1 reply; 12+ messages in thread
From: Kevin Wolf @ 2018-12-07 10:24 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-block, qemu-devel, mreitz
Am 07.12.2018 um 07:40 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > 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.
>
> I'd do this part in the next patch, when it has a user. Matter of
> taste.
I kept Fam as the author of the patches (except for the test case which
is effectively a rewrite with Fam's test as the spec), so I'd like to
do only the minimal editing necessary to make the patches mergeable. If
it's a matter of taste, I'll leave it as it is.
The same goes for code changes that are a matter of taste: If you
_really_ care about taste in the vmdk code, feel free to send a
follow-up and I'll merge it on top, but I'll leave Fam's patches alone
as much as I can.
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Thanks!
Kevin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] vmdk: Implement .bdrv_co_create callback
2018-12-07 7:10 ` Markus Armbruster
@ 2018-12-07 10:53 ` Kevin Wolf
2018-12-07 12:57 ` Markus Armbruster
0 siblings, 1 reply; 12+ messages in thread
From: Kevin Wolf @ 2018-12-07 10:53 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-block, qemu-devel, mreitz
Am 07.12.2018 um 08:10 hat Markus Armbruster geschrieben:
> This is a reasonably careful review of the QAPI-related parts, but more
> of an eye-over for the remainder.
>
> 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 | 1 +
> > block/vmdk.c | 452 ++++++++++++++++++++++++++++++------------
> > 3 files changed, 396 insertions(+), 127 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index d4fe710836..4778f88dd8 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4021,6 +4021,75 @@
> > 'size': 'size',
> > '*cluster-size' : 'size' } }
> >
> > +##
> > +# @BlockdevVmdkSubformat:
> > +#
> > +# Subformat options for VMDK images
> > +#
> > +# @monolithicSparse: Single file image with sparse cluster allocation
> > +#
> > +# @monolithicFlat: Single flat data image and a descriptor file
> > +#
> > +# @twoGbMaxExtentSparse: Data is split into 2GB (per virtual LBA) sparse extent
> > +# files, in addition to a descriptor file
> > +#
> > +# @twoGbMaxExtentFlat: Data is split into 2GB (per virtual LBA) flat extent
> > +# files, in addition to a descriptor file
> > +#
> > +# @streamOptimized: Single file image sparse cluster allocation, optimized
> > +# for streaming over network.
> > +#
> > +# Since: 4.0
> > +##
> > +{ 'enum': 'BlockdevVmdkSubformat',
> > + 'data': [ 'monolithicSparse', 'monolithicFlat', 'twoGbMaxExtentSparse',
> > + 'twoGbMaxExtentFlat', 'streamOptimized'] }
>
> Don't conform to the QAPI rules on names "to match VMDK spec spellings"
> (see qapi-schema.json below). We discussed this in review of v1.
>
> PRO: matches the VMDK spec (whatever that's worth), keeps the code
> simple.
>
> CON: the non-conforming names become part of the stable QMP interface,
> in the argument of command blockdev-create.
>
> I don't like the CON, but I'm willing to tolerate it in the name of
> simplicity.
I don't really have a opinion here. It seems this is what you had agreed
on in v1 and it still feels acceptable (even if not perfect) to you, so
I'll stick with it.
> > +
> > +##
> > +# @BlockdevVmdkAdapterType:
> > +#
> > +# Adapter type info for VMDK images
> > +#
> > +# Since: 4.0
> > +##
> > +{ 'enum': 'BlockdevVmdkAdapterType',
> > + 'data': [ 'ide', 'buslogic', 'lsilogic', 'legacyesx'] }
>
> May I have hyphens in the composite nouns? Hmm, might be the way they
> are to match VMDK spec spellings or for backward compatibility. I guess
> we'll see below.
These are literal values to be written into the VMDK descriptor files,
so they have to use this spelling. If we want to have a different
spelling in QMP, then we'll have to translate internally.
I guess this is the same argument as for BlockdevVmdkSubformat above.
For consistency, we should use the spec spellings in both cases or not
at all.
> > +
> > +##
> > +# @BlockdevCreateOptionsVmdk:
> > +#
> > +# Driver specific image creation options for VMDK.
> > +#
> > +# @file Where to store the new image file. This refers to the image
> > +# file for monolithcSparse and streamOptimized format, or the
> > +# descriptor file for other formats.
> > +# @size Size of the virtual disk in bytes
> > +# @extents Where to store the data extents. Required for monolithcflat,
> > +# twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For
> > +# monolithicflat, only one entry is required; for
>
> monolithicFlat
>
> > +# twoGbMaxExtent* formats, the number of entries required is
> > +# calculated as extent_number = virtual_size / 2GB.
>
> Is it okay to supply more entries than required, or do I have to supply
> exactly the right number?
If I understand the code correctly, additional extents are silently
ignored.
> > +# @subformat The subformat of the VMDK image. Default: "monolithicsparse".
>
> monolithicSparse
>
> > +# @backing-file The path of backing file. Default: no backing file is used.
> > +# @adapter-type The adapter type used to fill in the descriptor. Default: ide.
> > +# @hwversion Hardware version. The meaningful options are "4" or "6".
> > +# Defaulted to "4".
>
> Default: "4"
Fixed all the spelling changes.
> > +# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats.
> > +# Default: false.
> > +#
> > +# Since: 4.0
> > +##
> > +{ 'struct': 'BlockdevCreateOptionsVmdk',
> > + 'data': { 'file': 'BlockdevRef',
> > + 'size': 'size',
> > + '*extents': ['BlockdevRef'],
> > + '*subformat': 'BlockdevVmdkSubformat',
> > + '*backing-file': 'str',
> > + '*adapter-type': 'BlockdevVmdkAdapterType',
> > + '*hwversion': 'str',
> > + '*zeroed-grain': 'bool' } }
>
> @zeroed-grain is undocumented.
It's right there at the beginning of the quote after your last comment?
> > +
> > +
> > ##
> > # @SheepdogRedundancyType:
> > #
> > @@ -4215,6 +4284,7 @@
> > 'ssh': 'BlockdevCreateOptionsSsh',
> > 'vdi': 'BlockdevCreateOptionsVdi',
> > 'vhdx': 'BlockdevCreateOptionsVhdx',
> > + 'vmdk': 'BlockdevCreateOptionsVmdk',
> > 'vpc': 'BlockdevCreateOptionsVpc'
> > } }
> >
> > diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> > index 65b6dc2f6f..78e8bcd561 100644
> > --- a/qapi/qapi-schema.json
> > +++ b/qapi/qapi-schema.json
> > @@ -66,6 +66,7 @@
> > 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status
> > 'CpuInfoMIPS', # PC, visible through query-cpu
> > 'CpuInfoTricore', # PC, visible through query-cpu
> > + 'BlockdevVmdkSubformat',# all members, to match VMDK spec spellings
> > 'QapiErrorClass', # all members, visible through errors
> > 'UuidInfo', # UUID, visible through query-uuid
> > 'X86CPURegister32', # all members, visible indirectly through qom-get
> > diff --git a/block/vmdk.c b/block/vmdk.c
> > index 32fc2c84b3..16f86457d7 100644
> > --- a/block/vmdk.c
> > +++ b/block/vmdk.c
> > @@ -1932,33 +1932,56 @@ static int filename_decompose(const char *filename, char *path, char *prefix,
> > return VMDK_OK;
> > }
> >
> > -static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
> > - Error **errp)
> > +/*
> > + * idx == 0: get or create the descriptor file (also the image file if in a
> > + * non-split format.
> > + * idx >= 1: get the n-th extent if in a split subformat
> > + */
> > +typedef BlockBackend *(*vmdk_create_extent_fn)(int64_t size,
> > + int idx,
> > + bool flat,
> > + bool split,
> > + bool compress,
> > + bool zeroed_grain,
> > + void *opaque,
> > + Error **errp);
> > +
> > +static void vmdk_desc_add_extent(GString *desc,
> > + const char *extent_line_fmt,
> > + int64_t size, const char *filename)
> > +{
> > + char *basename = g_path_get_basename(filename);
>
> I like a blank line between declarations and statements.
Matter of taste. But okay, I already changed this function, so I'll give
you this one.
> > + g_string_append_printf(desc, extent_line_fmt,
> > + DIV_ROUND_UP(size, BDRV_SECTOR_SIZE), basename);
> > +
> > + g_free(basename);
> > +}
> > +
> > +static int coroutine_fn vmdk_co_do_create(int64_t size,
> > + BlockdevVmdkSubformat subformat,
> > + BlockdevVmdkAdapterType adapter_type,
> > + const char *backing_file,
> > + const char *hw_version,
> > + bool compat6,
> > + bool zeroed_grain,
> > + vmdk_create_extent_fn extent_fn,
> > + void *opaque,
> > + Error **errp)
> > {
> > - int idx = 0;
> > - BlockBackend *new_blk = NULL;
> > + int extent_idx;
> > + BlockBackend *blk = NULL;
> > Error *local_err = NULL;
> > char *desc = NULL;
> > - int64_t total_size = 0, filesize;
> > - char *adapter_type = NULL;
> > - char *backing_file = NULL;
> > - char *hw_version = NULL;
> > - char *fmt = NULL;
> > int ret = 0;
> > bool flat, split, compress;
> > GString *ext_desc_lines;
> > - char *path = g_malloc0(PATH_MAX);
> > - char *prefix = g_malloc0(PATH_MAX);
> > - char *postfix = g_malloc0(PATH_MAX);
> > - char *desc_line = g_malloc0(BUF_SIZE);
> > - char *ext_filename = g_malloc0(PATH_MAX);
> > - char *desc_filename = g_malloc0(PATH_MAX);
> > const int64_t split_size = 0x80000000; /* VMDK has constant split size */
> > - const char *desc_extent_line;
> > + int64_t extent_size;
> > + int64_t created_size = 0;
> > + const char *extent_line_fmt;
> > char *parent_desc_line = g_malloc0(BUF_SIZE);
> > uint32_t parent_cid = 0xffffffff;
> > uint32_t number_heads = 16;
> > - bool zeroed_grain = false;
> > uint32_t desc_offset = 0, desc_len;
> > const char desc_template[] =
> > "# Disk DescriptorFile\n"
> > @@ -1982,71 +2005,35 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
> >
> > ext_desc_lines = g_string_new(NULL);
> >
> > - if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
> > - ret = -EINVAL;
> > - goto exit;
> > - }
> > /* Read out options */
> > - total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> > - BDRV_SECTOR_SIZE);
> > - adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
> > - backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> > - hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION);
> > - if (qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false)) {
> > - if (strcmp(hw_version, "undefined")) {
> > + if (compat6) {
> > + if (hw_version) {
> > error_setg(errp,
> > "compat6 cannot be enabled with hwversion set");
> > ret = -EINVAL;
> > goto exit;
> > }
> > - g_free(hw_version);
> > - hw_version = g_strdup("6");
> > - }
> > - if (strcmp(hw_version, "undefined") == 0) {
> > - g_free(hw_version);
> > - hw_version = g_strdup("4");
> > + hw_version = "6";
> > }
> > - fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
> > - if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false)) {
> > - zeroed_grain = true;
> > + if (!hw_version) {
> > + hw_version = "4";
> > }
> >
> > - if (!adapter_type) {
> > - adapter_type = g_strdup("ide");
> > - } else if (strcmp(adapter_type, "ide") &&
> > - strcmp(adapter_type, "buslogic") &&
> > - strcmp(adapter_type, "lsilogic") &&
> > - strcmp(adapter_type, "legacyESX")) {
> > - error_setg(errp, "Unknown adapter type: '%s'", adapter_type);
> > - ret = -EINVAL;
> > - goto exit;
> > - }
>
> Old code recognizes "legacyESX". New code recognizes "legacyesx". Bug
> or feature?
Good catch!
Who knows? But it's not advertised as a feature in the commit message,
and I can only see the spelling "legacyESX" in the spec, so I'll change
the QAPI definition to "legacyESX". Leaving things as they were should
be safe.
Fam also made the value case insensitive when parsing QemuOpts in
vmdk_co_create_opts() to compensate for the change. I think this can
also go away then?
Kevin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] vmdk: Refactor vmdk_create_extent
2018-12-07 10:24 ` Kevin Wolf
@ 2018-12-07 12:52 ` Markus Armbruster
0 siblings, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2018-12-07 12:52 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz
Kevin Wolf <kwolf@redhat.com> writes:
> Am 07.12.2018 um 07:40 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > 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.
>>
>> I'd do this part in the next patch, when it has a user. Matter of
>> taste.
>
> I kept Fam as the author of the patches (except for the test case which
> is effectively a rewrite with Fam's test as the spec), so I'd like to
> do only the minimal editing necessary to make the patches mergeable. If
> it's a matter of taste, I'll leave it as it is.
That's fine. Matter of taste means I'm stating mine without demanding
you change the patch to conform to it.
> The same goes for code changes that are a matter of taste: If you
> _really_ care about taste in the vmdk code, feel free to send a
> follow-up and I'll merge it on top, but I'll leave Fam's patches alone
> as much as I can.
Makes sense.
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks!
>
> Kevin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] vmdk: Implement .bdrv_co_create callback
2018-12-07 10:53 ` Kevin Wolf
@ 2018-12-07 12:57 ` Markus Armbruster
0 siblings, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2018-12-07 12:57 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz
Kevin Wolf <kwolf@redhat.com> writes:
> Am 07.12.2018 um 08:10 hat Markus Armbruster geschrieben:
>> This is a reasonably careful review of the QAPI-related parts, but more
>> of an eye-over for the remainder.
>>
>> 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 | 1 +
>> > block/vmdk.c | 452 ++++++++++++++++++++++++++++++------------
>> > 3 files changed, 396 insertions(+), 127 deletions(-)
>> >
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index d4fe710836..4778f88dd8 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -4021,6 +4021,75 @@
>> > 'size': 'size',
>> > '*cluster-size' : 'size' } }
>> >
>> > +##
>> > +# @BlockdevVmdkSubformat:
>> > +#
>> > +# Subformat options for VMDK images
>> > +#
>> > +# @monolithicSparse: Single file image with sparse cluster allocation
>> > +#
>> > +# @monolithicFlat: Single flat data image and a descriptor file
>> > +#
>> > +# @twoGbMaxExtentSparse: Data is split into 2GB (per virtual LBA) sparse extent
>> > +# files, in addition to a descriptor file
>> > +#
>> > +# @twoGbMaxExtentFlat: Data is split into 2GB (per virtual LBA) flat extent
>> > +# files, in addition to a descriptor file
>> > +#
>> > +# @streamOptimized: Single file image sparse cluster allocation, optimized
>> > +# for streaming over network.
>> > +#
>> > +# Since: 4.0
>> > +##
>> > +{ 'enum': 'BlockdevVmdkSubformat',
>> > + 'data': [ 'monolithicSparse', 'monolithicFlat', 'twoGbMaxExtentSparse',
>> > + 'twoGbMaxExtentFlat', 'streamOptimized'] }
>>
>> Don't conform to the QAPI rules on names "to match VMDK spec spellings"
>> (see qapi-schema.json below). We discussed this in review of v1.
>>
>> PRO: matches the VMDK spec (whatever that's worth), keeps the code
>> simple.
>>
>> CON: the non-conforming names become part of the stable QMP interface,
>> in the argument of command blockdev-create.
>>
>> I don't like the CON, but I'm willing to tolerate it in the name of
>> simplicity.
>
> I don't really have a opinion here. It seems this is what you had agreed
> on in v1 and it still feels acceptable (even if not perfect) to you, so
> I'll stick with it.
That's okay. I think I'd like the alternatives less.
>> > +
>> > +##
>> > +# @BlockdevVmdkAdapterType:
>> > +#
>> > +# Adapter type info for VMDK images
>> > +#
>> > +# Since: 4.0
>> > +##
>> > +{ 'enum': 'BlockdevVmdkAdapterType',
>> > + 'data': [ 'ide', 'buslogic', 'lsilogic', 'legacyesx'] }
>>
>> May I have hyphens in the composite nouns? Hmm, might be the way they
>> are to match VMDK spec spellings or for backward compatibility. I guess
>> we'll see below.
>
> These are literal values to be written into the VMDK descriptor files,
> so they have to use this spelling. If we want to have a different
> spelling in QMP, then we'll have to translate internally.
>
> I guess this is the same argument as for BlockdevVmdkSubformat above.
> For consistency, we should use the spec spellings in both cases or not
> at all.
Makes sense.
>> > +
>> > +##
>> > +# @BlockdevCreateOptionsVmdk:
>> > +#
>> > +# Driver specific image creation options for VMDK.
>> > +#
>> > +# @file Where to store the new image file. This refers to the image
>> > +# file for monolithcSparse and streamOptimized format, or the
>> > +# descriptor file for other formats.
>> > +# @size Size of the virtual disk in bytes
>> > +# @extents Where to store the data extents. Required for monolithcflat,
>> > +# twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For
>> > +# monolithicflat, only one entry is required; for
>>
>> monolithicFlat
>>
>> > +# twoGbMaxExtent* formats, the number of entries required is
>> > +# calculated as extent_number = virtual_size / 2GB.
>>
>> Is it okay to supply more entries than required, or do I have to supply
>> exactly the right number?
>
> If I understand the code correctly, additional extents are silently
> ignored.
I see you're fixing that in v4. Good.
>> > +# @subformat The subformat of the VMDK image. Default: "monolithicsparse".
>>
>> monolithicSparse
>>
>> > +# @backing-file The path of backing file. Default: no backing file is used.
>> > +# @adapter-type The adapter type used to fill in the descriptor. Default: ide.
>> > +# @hwversion Hardware version. The meaningful options are "4" or "6".
>> > +# Defaulted to "4".
>>
>> Default: "4"
>
> Fixed all the spelling changes.
>
>> > +# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats.
>> > +# Default: false.
>> > +#
>> > +# Since: 4.0
>> > +##
>> > +{ 'struct': 'BlockdevCreateOptionsVmdk',
>> > + 'data': { 'file': 'BlockdevRef',
>> > + 'size': 'size',
>> > + '*extents': ['BlockdevRef'],
>> > + '*subformat': 'BlockdevVmdkSubformat',
>> > + '*backing-file': 'str',
>> > + '*adapter-type': 'BlockdevVmdkAdapterType',
>> > + '*hwversion': 'str',
>> > + '*zeroed-grain': 'bool' } }
>>
>> @zeroed-grain is undocumented.
>
> It's right there at the beginning of the quote after your last comment?
You're right, sorry for the noise.
>> > +
>> > +
>> > ##
>> > # @SheepdogRedundancyType:
>> > #
>> > @@ -4215,6 +4284,7 @@
>> > 'ssh': 'BlockdevCreateOptionsSsh',
>> > 'vdi': 'BlockdevCreateOptionsVdi',
>> > 'vhdx': 'BlockdevCreateOptionsVhdx',
>> > + 'vmdk': 'BlockdevCreateOptionsVmdk',
>> > 'vpc': 'BlockdevCreateOptionsVpc'
>> > } }
>> >
>> > diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
>> > index 65b6dc2f6f..78e8bcd561 100644
>> > --- a/qapi/qapi-schema.json
>> > +++ b/qapi/qapi-schema.json
>> > @@ -66,6 +66,7 @@
>> > 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status
>> > 'CpuInfoMIPS', # PC, visible through query-cpu
>> > 'CpuInfoTricore', # PC, visible through query-cpu
>> > + 'BlockdevVmdkSubformat',# all members, to match VMDK spec spellings
>> > 'QapiErrorClass', # all members, visible through errors
>> > 'UuidInfo', # UUID, visible through query-uuid
>> > 'X86CPURegister32', # all members, visible indirectly through qom-get
>> > diff --git a/block/vmdk.c b/block/vmdk.c
>> > index 32fc2c84b3..16f86457d7 100644
>> > --- a/block/vmdk.c
>> > +++ b/block/vmdk.c
>> > @@ -1932,33 +1932,56 @@ static int filename_decompose(const char *filename, char *path, char *prefix,
>> > return VMDK_OK;
>> > }
>> >
>> > -static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
>> > - Error **errp)
>> > +/*
>> > + * idx == 0: get or create the descriptor file (also the image file if in a
>> > + * non-split format.
>> > + * idx >= 1: get the n-th extent if in a split subformat
>> > + */
>> > +typedef BlockBackend *(*vmdk_create_extent_fn)(int64_t size,
>> > + int idx,
>> > + bool flat,
>> > + bool split,
>> > + bool compress,
>> > + bool zeroed_grain,
>> > + void *opaque,
>> > + Error **errp);
>> > +
>> > +static void vmdk_desc_add_extent(GString *desc,
>> > + const char *extent_line_fmt,
>> > + int64_t size, const char *filename)
>> > +{
>> > + char *basename = g_path_get_basename(filename);
>>
>> I like a blank line between declarations and statements.
>
> Matter of taste. But okay, I already changed this function, so I'll give
> you this one.
>
>> > + g_string_append_printf(desc, extent_line_fmt,
>> > + DIV_ROUND_UP(size, BDRV_SECTOR_SIZE), basename);
>> > +
>> > + g_free(basename);
>> > +}
>> > +
>> > +static int coroutine_fn vmdk_co_do_create(int64_t size,
>> > + BlockdevVmdkSubformat subformat,
>> > + BlockdevVmdkAdapterType adapter_type,
>> > + const char *backing_file,
>> > + const char *hw_version,
>> > + bool compat6,
>> > + bool zeroed_grain,
>> > + vmdk_create_extent_fn extent_fn,
>> > + void *opaque,
>> > + Error **errp)
>> > {
>> > - int idx = 0;
>> > - BlockBackend *new_blk = NULL;
>> > + int extent_idx;
>> > + BlockBackend *blk = NULL;
>> > Error *local_err = NULL;
>> > char *desc = NULL;
>> > - int64_t total_size = 0, filesize;
>> > - char *adapter_type = NULL;
>> > - char *backing_file = NULL;
>> > - char *hw_version = NULL;
>> > - char *fmt = NULL;
>> > int ret = 0;
>> > bool flat, split, compress;
>> > GString *ext_desc_lines;
>> > - char *path = g_malloc0(PATH_MAX);
>> > - char *prefix = g_malloc0(PATH_MAX);
>> > - char *postfix = g_malloc0(PATH_MAX);
>> > - char *desc_line = g_malloc0(BUF_SIZE);
>> > - char *ext_filename = g_malloc0(PATH_MAX);
>> > - char *desc_filename = g_malloc0(PATH_MAX);
>> > const int64_t split_size = 0x80000000; /* VMDK has constant split size */
>> > - const char *desc_extent_line;
>> > + int64_t extent_size;
>> > + int64_t created_size = 0;
>> > + const char *extent_line_fmt;
>> > char *parent_desc_line = g_malloc0(BUF_SIZE);
>> > uint32_t parent_cid = 0xffffffff;
>> > uint32_t number_heads = 16;
>> > - bool zeroed_grain = false;
>> > uint32_t desc_offset = 0, desc_len;
>> > const char desc_template[] =
>> > "# Disk DescriptorFile\n"
>> > @@ -1982,71 +2005,35 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
>> >
>> > ext_desc_lines = g_string_new(NULL);
>> >
>> > - if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) {
>> > - ret = -EINVAL;
>> > - goto exit;
>> > - }
>> > /* Read out options */
>> > - total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
>> > - BDRV_SECTOR_SIZE);
>> > - adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
>> > - backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
>> > - hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION);
>> > - if (qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false)) {
>> > - if (strcmp(hw_version, "undefined")) {
>> > + if (compat6) {
>> > + if (hw_version) {
>> > error_setg(errp,
>> > "compat6 cannot be enabled with hwversion set");
>> > ret = -EINVAL;
>> > goto exit;
>> > }
>> > - g_free(hw_version);
>> > - hw_version = g_strdup("6");
>> > - }
>> > - if (strcmp(hw_version, "undefined") == 0) {
>> > - g_free(hw_version);
>> > - hw_version = g_strdup("4");
>> > + hw_version = "6";
>> > }
>> > - fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
>> > - if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false)) {
>> > - zeroed_grain = true;
>> > + if (!hw_version) {
>> > + hw_version = "4";
>> > }
>> >
>> > - if (!adapter_type) {
>> > - adapter_type = g_strdup("ide");
>> > - } else if (strcmp(adapter_type, "ide") &&
>> > - strcmp(adapter_type, "buslogic") &&
>> > - strcmp(adapter_type, "lsilogic") &&
>> > - strcmp(adapter_type, "legacyESX")) {
>> > - error_setg(errp, "Unknown adapter type: '%s'", adapter_type);
>> > - ret = -EINVAL;
>> > - goto exit;
>> > - }
>>
>> Old code recognizes "legacyESX". New code recognizes "legacyesx". Bug
>> or feature?
>
> Good catch!
>
> Who knows? But it's not advertised as a feature in the commit message,
> and I can only see the spelling "legacyESX" in the spec, so I'll change
> the QAPI definition to "legacyESX". Leaving things as they were should
> be safe.
I suspect it's a remnant of v1, where Fam used all lower case enum
values, plus code to match them case insensitively.
> Fam also made the value case insensitive when parsing QemuOpts in
> vmdk_co_create_opts() to compensate for the change. I think this can
> also go away then?
Yes, please.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-12-07 12:57 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-06 15:13 [Qemu-devel] [PATCH v3 0/4] vmdk: Implement blockdev-create Kevin Wolf
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 1/4] vmdk: Refactor vmdk_create_extent Kevin Wolf
2018-12-07 6:40 ` Markus Armbruster
2018-12-07 10:24 ` Kevin Wolf
2018-12-07 12:52 ` Markus Armbruster
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 2/4] vmdk: Implement .bdrv_co_create callback Kevin Wolf
2018-12-07 7:10 ` Markus Armbruster
2018-12-07 10:53 ` Kevin Wolf
2018-12-07 12:57 ` Markus Armbruster
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 3/4] iotests: Filter cid numbers in VMDK extent info Kevin Wolf
2018-12-06 15:13 ` [Qemu-devel] [PATCH v3 4/4] iotests: Add VMDK tests for blockdev-create Kevin Wolf
2018-12-06 20:10 ` [Qemu-devel] [PATCH v3 0/4] vmdk: Implement blockdev-create no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.