The following changes since commit b2f7a038bb4c4fc5ce6b8486e8513dfd97665e2a: Merge remote-tracking branch 'remotes/rth/tags/pull-softfloat-20181104' into staging (2018-11-05 10:32:49 +0000) are available in the Git repository at: git://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to 1240ac558d348f6c7a5752b1a57c1da58e4efe3e: include: Add a comment to explain the origin of sizes' lookup table (2018-11-05 15:29:59 +0100) ---------------------------------------------------------------- Block layer patches: - auto-read-only option to fix commit job when used with -blockdev - Fix help text related qemu-iotests failure (by improving the help text and updating the reference output) - quorum: Add missing checks when adding/removing child nodes - Don't take address of fields in packed structs - vvfat: Fix crash when reporting error about too many files in directory ---------------------------------------------------------------- Alberto Garcia (7): block: replace "discard" literal with BDRV_OPT_DISCARD macro qcow2: Get the request alignment for encrypted images from QCryptoBlock quorum: Remove quorum_err() quorum: Return an error if the blkverify mode has invalid settings iotest: Test the blkverify mode of the Quorum driver quorum: Forbid adding children in blkverify mode iotest: Test x-blockdev-change on a Quorum Cleber Rosa (1): iotests: make 083 specific to raw Daniel P. Berrangé (1): crypto: initialize sector size even when opening with no IO flag Kevin Wolf (12): vpc: Don't leak opts in vpc_open() block: Update flags in bdrv_set_read_only() block: Add auto-read-only option rbd: Close image in qemu_rbd_open() error path block: Require auto-read-only for existing fallbacks nbd: Support auto-read-only option file-posix: Support auto-read-only option curl: Support auto-read-only option gluster: Support auto-read-only option iscsi: Support auto-read-only option block: Make auto-read-only=on default for -drive qemu-iotests: Test auto-read-only with -drive and -blockdev Leonid Bloch (2): vdi: Use a literal number of bytes for DEFAULT_CLUSTER_SIZE include: Add a comment to explain the origin of sizes' lookup table Li Qiang (1): block: change some function return type to bool Max Reitz (5): option: Make option help nicer to read chardev: Indent list of chardevs qdev-monitor: Make device options help nicer object: Make option help nicer to read fw_cfg: Drop newline in @file description Peter Maydell (5): block/qcow2: Don't take address of fields in packed structs block/qcow: Don't take address of fields in packed structs block/qcow2-bitmap: Don't take address of fields in packed structs block/vhdx: Don't take address of fields in packed structs block/vdi: Don't take address of fields in packed structs Stefan Weil (1): qemu-io-cmds: Fix two format strings Thomas Huth (1): block/vvfat: Fix crash when reporting error about too many files in directory qapi/block-core.json | 7 + block/vhdx.h | 12 +- include/block/block.h | 5 +- include/qemu/option.h | 2 +- include/qemu/units.h | 18 + include/sysemu/block-backend.h | 6 +- block.c | 60 ++- block/block-backend.c | 8 +- block/bochs.c | 17 +- block/cloop.c | 16 +- block/curl.c | 8 +- block/dmg.c | 16 +- block/file-posix.c | 19 +- block/gluster.c | 12 +- block/iscsi.c | 8 +- block/nbd-client.c | 10 +- block/qcow.c | 18 +- block/qcow2-bitmap.c | 24 +- block/qcow2.c | 66 +-- block/quorum.c | 45 +- block/rbd.c | 14 +- block/vdi.c | 68 +-- block/vhdx-endian.c | 118 ++--- block/vhdx-log.c | 4 +- block/vhdx.c | 18 +- block/vpc.c | 2 + block/vvfat.c | 15 +- blockdev.c | 3 +- chardev/char.c | 2 +- crypto/block-qcow.c | 2 + qdev-monitor.c | 13 +- qemu-img.c | 4 +- qemu-io-cmds.c | 4 +- util/qemu-option.c | 32 +- vl.c | 15 +- tests/qemu-iotests/081 | 116 +++++ tests/qemu-iotests/081.out | 70 +++ tests/qemu-iotests/082.out | 956 ++++++++++++++++++++--------------------- tests/qemu-iotests/083 | 2 +- tests/qemu-iotests/232 | 147 +++++++ tests/qemu-iotests/232.out | 59 +++ tests/qemu-iotests/group | 1 + 42 files changed, 1266 insertions(+), 776 deletions(-) create mode 100755 tests/qemu-iotests/232 create mode 100644 tests/qemu-iotests/232.out From: Thomas Huth <thuth@redhat.com> When using the vvfat driver with a directory that contains too many files, QEMU currently crashes. This can be triggered like this for example: mkdir /tmp/vvfattest cd /tmp/vvfattest for ((x=0;x<=513;x++)); do mkdir $x; done qemu-system-x86_64 -drive \ file.driver=vvfat,file.dir=.,read-only=on,media=cdrom Seems like read_directory() is changing the mapping->path variable. Make sure we use the right pointer instead. Signed-off-by: Thomas Huth <thuth@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vvfat.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block/vvfat.c b/block/vvfat.c index XXXXXXX..XXXXXXX 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -XXX,XX +XXX,XX @@ static int init_directories(BDRVVVFATState* s, mapping = array_get(&(s->mapping), i); if (mapping->mode & MODE_DIRECTORY) { + char *path = mapping->path; mapping->begin = cluster; if(read_directory(s, i)) { - error_setg(errp, "Could not read directory %s", - mapping->path); + error_setg(errp, "Could not read directory %s", path); return -1; } mapping = array_get(&(s->mapping), i); -- 2.19.1 From: Alberto Garcia <berto@igalia.com> Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ QemuOptsList bdrv_runtime_opts = { .help = "try to optimize zero writes (off, on, unmap)", }, { - .name = "discard", + .name = BDRV_OPT_DISCARD, .type = QEMU_OPT_STRING, .help = "discard operation (ignore/off, unmap/on)", }, @@ -XXX,XX +XXX,XX @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file, } } - discard = qemu_opt_get(opts, "discard"); + discard = qemu_opt_get(opts, BDRV_OPT_DISCARD); if (discard != NULL) { if (bdrv_parse_discard_flags(discard, &bs->open_flags) != 0) { error_setg(errp, "Invalid discard option"); @@ -XXX,XX +XXX,XX @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, update_flags_from_options(&reopen_state->flags, opts); - discard = qemu_opt_get_del(opts, "discard"); + discard = qemu_opt_get_del(opts, BDRV_OPT_DISCARD); if (discard != NULL) { if (bdrv_parse_discard_flags(discard, &reopen_state->flags) != 0) { error_setg(errp, "Invalid discard option"); -- 2.19.1 From: Stefan Weil <sw@weilnetz.de> Use %zu instead of %zd for unsigned numbers. This fixes two error messages from the LSTM static code analyzer: This argument should be of type 'ssize_t' but is of type 'unsigned long' Signed-off-by: Stefan Weil <sw@weilnetz.de> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qemu-io-cmds.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index XXXXXXX..XXXXXXX 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -XXX,XX +XXX,XX @@ static int readv_f(BlockBackend *blk, int argc, char **argv) memset(cmp_buf, pattern, qiov.size); if (memcmp(buf, cmp_buf, qiov.size)) { printf("Pattern verification failed at offset %" - PRId64 ", %zd bytes\n", offset, qiov.size); + PRId64 ", %zu bytes\n", offset, qiov.size); ret = -EINVAL; } g_free(cmp_buf); @@ -XXX,XX +XXX,XX @@ static void aio_read_done(void *opaque, int ret) memset(cmp_buf, ctx->pattern, ctx->qiov.size); if (memcmp(ctx->buf, cmp_buf, ctx->qiov.size)) { printf("Pattern verification failed at offset %" - PRId64 ", %zd bytes\n", ctx->offset, ctx->qiov.size); + PRId64 ", %zu bytes\n", ctx->offset, ctx->qiov.size); } g_free(cmp_buf); } -- 2.19.1 From: Peter Maydell <peter.maydell@linaro.org> Taking the address of a field in a packed struct is a bad idea, because it might not be actually aligned enough for that pointer type (and thus cause a crash on dereference on some host architectures). Newer versions of clang warn about this. Avoid the bug by not using the "modify in place" byte swapping functions. There are a few places where the in-place swap function is used on something other than a packed struct field; we convert those anyway, for consistency. This patch was produced with the following spatch script (and hand-editing to fold a few resulting overlength lines): @@ expression E; @@ -be16_to_cpus(&E); +E = be16_to_cpu(E); @@ expression E; @@ -be32_to_cpus(&E); +E = be32_to_cpu(E); @@ expression E; @@ -be64_to_cpus(&E); +E = be64_to_cpu(E); @@ expression E; @@ -cpu_to_be16s(&E); +E = cpu_to_be16(E); @@ expression E; @@ -cpu_to_be32s(&E); +E = cpu_to_be32(E); @@ expression E; @@ -cpu_to_be64s(&E); +E = cpu_to_be64(E); Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Tested-by: John Snow <jsnow@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2.c | 64 +++++++++++++++++++++++++++------------------------ 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, "pread fail from offset %" PRIu64, offset); return 1; } - be32_to_cpus(&ext.magic); - be32_to_cpus(&ext.len); + ext.magic = be32_to_cpu(ext.magic); + ext.len = be32_to_cpu(ext.len); offset += sizeof(ext); #ifdef DEBUG_EXT printf("ext.magic = 0x%x\n", ext.magic); @@ -XXX,XX +XXX,XX @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, "Unable to read CRYPTO header extension"); return ret; } - be64_to_cpus(&s->crypto_header.offset); - be64_to_cpus(&s->crypto_header.length); + s->crypto_header.offset = be64_to_cpu(s->crypto_header.offset); + s->crypto_header.length = be64_to_cpu(s->crypto_header.length); if ((s->crypto_header.offset % s->cluster_size) != 0) { error_setg(errp, "Encryption header offset '%" PRIu64 "' is " @@ -XXX,XX +XXX,XX @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, return -EINVAL; } - be32_to_cpus(&bitmaps_ext.nb_bitmaps); - be64_to_cpus(&bitmaps_ext.bitmap_directory_size); - be64_to_cpus(&bitmaps_ext.bitmap_directory_offset); + bitmaps_ext.nb_bitmaps = be32_to_cpu(bitmaps_ext.nb_bitmaps); + bitmaps_ext.bitmap_directory_size = + be64_to_cpu(bitmaps_ext.bitmap_directory_size); + bitmaps_ext.bitmap_directory_offset = + be64_to_cpu(bitmaps_ext.bitmap_directory_offset); if (bitmaps_ext.nb_bitmaps > QCOW2_MAX_BITMAPS) { error_setg(errp, @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, error_setg_errno(errp, -ret, "Could not read qcow2 header"); goto fail; } - be32_to_cpus(&header.magic); - be32_to_cpus(&header.version); - be64_to_cpus(&header.backing_file_offset); - be32_to_cpus(&header.backing_file_size); - be64_to_cpus(&header.size); - be32_to_cpus(&header.cluster_bits); - be32_to_cpus(&header.crypt_method); - be64_to_cpus(&header.l1_table_offset); - be32_to_cpus(&header.l1_size); - be64_to_cpus(&header.refcount_table_offset); - be32_to_cpus(&header.refcount_table_clusters); - be64_to_cpus(&header.snapshots_offset); - be32_to_cpus(&header.nb_snapshots); + header.magic = be32_to_cpu(header.magic); + header.version = be32_to_cpu(header.version); + header.backing_file_offset = be64_to_cpu(header.backing_file_offset); + header.backing_file_size = be32_to_cpu(header.backing_file_size); + header.size = be64_to_cpu(header.size); + header.cluster_bits = be32_to_cpu(header.cluster_bits); + header.crypt_method = be32_to_cpu(header.crypt_method); + header.l1_table_offset = be64_to_cpu(header.l1_table_offset); + header.l1_size = be32_to_cpu(header.l1_size); + header.refcount_table_offset = be64_to_cpu(header.refcount_table_offset); + header.refcount_table_clusters = + be32_to_cpu(header.refcount_table_clusters); + header.snapshots_offset = be64_to_cpu(header.snapshots_offset); + header.nb_snapshots = be32_to_cpu(header.nb_snapshots); if (header.magic != QCOW_MAGIC) { error_setg(errp, "Image is not in qcow2 format"); @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, header.refcount_order = 4; header.header_length = 72; } else { - be64_to_cpus(&header.incompatible_features); - be64_to_cpus(&header.compatible_features); - be64_to_cpus(&header.autoclear_features); - be32_to_cpus(&header.refcount_order); - be32_to_cpus(&header.header_length); + header.incompatible_features = + be64_to_cpu(header.incompatible_features); + header.compatible_features = be64_to_cpu(header.compatible_features); + header.autoclear_features = be64_to_cpu(header.autoclear_features); + header.refcount_order = be32_to_cpu(header.refcount_order); + header.header_length = be32_to_cpu(header.header_length); if (header.header_length < 104) { error_setg(errp, "qcow2 header too short"); @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, goto fail; } for(i = 0;i < s->l1_size; i++) { - be64_to_cpus(&s->l1_table[i]); + s->l1_table[i] = be64_to_cpu(s->l1_table[i]); } } @@ -XXX,XX +XXX,XX @@ int qcow2_update_header(BlockDriverState *bs) /* Full disk encryption header pointer extension */ if (s->crypto_header.offset != 0) { - cpu_to_be64s(&s->crypto_header.offset); - cpu_to_be64s(&s->crypto_header.length); + s->crypto_header.offset = cpu_to_be64(s->crypto_header.offset); + s->crypto_header.length = cpu_to_be64(s->crypto_header.length); ret = header_ext_add(buf, QCOW2_EXT_MAGIC_CRYPTO_HEADER, &s->crypto_header, sizeof(s->crypto_header), buflen); - be64_to_cpus(&s->crypto_header.offset); - be64_to_cpus(&s->crypto_header.length); + s->crypto_header.offset = be64_to_cpu(s->crypto_header.offset); + s->crypto_header.length = be64_to_cpu(s->crypto_header.length); if (ret < 0) { goto fail; } -- 2.19.1 From: Peter Maydell <peter.maydell@linaro.org> Taking the address of a field in a packed struct is a bad idea, because it might not be actually aligned enough for that pointer type (and thus cause a crash on dereference on some host architectures). Newer versions of clang warn about this. Avoid the bug by not using the "modify in place" byte swapping functions. There are a few places where the in-place swap function is used on something other than a packed struct field; we convert those anyway, for consistency. This patch was produced with the following spatch script: @@ expression E; @@ -be16_to_cpus(&E); +E = be16_to_cpu(E); @@ expression E; @@ -be32_to_cpus(&E); +E = be32_to_cpu(E); @@ expression E; @@ -be64_to_cpus(&E); +E = be64_to_cpu(E); @@ expression E; @@ -cpu_to_be16s(&E); +E = cpu_to_be16(E); @@ expression E; @@ -cpu_to_be32s(&E); +E = cpu_to_be32(E); @@ expression E; @@ -cpu_to_be64s(&E); +E = cpu_to_be64(E); Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Tested-by: John Snow <jsnow@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/block/qcow.c b/block/qcow.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -XXX,XX +XXX,XX @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, if (ret < 0) { goto fail; } - be32_to_cpus(&header.magic); - be32_to_cpus(&header.version); - be64_to_cpus(&header.backing_file_offset); - be32_to_cpus(&header.backing_file_size); - be32_to_cpus(&header.mtime); - be64_to_cpus(&header.size); - be32_to_cpus(&header.crypt_method); - be64_to_cpus(&header.l1_table_offset); + header.magic = be32_to_cpu(header.magic); + header.version = be32_to_cpu(header.version); + header.backing_file_offset = be64_to_cpu(header.backing_file_offset); + header.backing_file_size = be32_to_cpu(header.backing_file_size); + header.mtime = be32_to_cpu(header.mtime); + header.size = be64_to_cpu(header.size); + header.crypt_method = be32_to_cpu(header.crypt_method); + header.l1_table_offset = be64_to_cpu(header.l1_table_offset); if (header.magic != QCOW_MAGIC) { error_setg(errp, "Image not in qcow format"); @@ -XXX,XX +XXX,XX @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, } for(i = 0;i < s->l1_size; i++) { - be64_to_cpus(&s->l1_table[i]); + s->l1_table[i] = be64_to_cpu(s->l1_table[i]); } /* alloc L2 cache (max. 64k * 16 * 8 = 8 MB) */ -- 2.19.1 From: Peter Maydell <peter.maydell@linaro.org> Taking the address of a field in a packed struct is a bad idea, because it might not be actually aligned enough for that pointer type (and thus cause a crash on dereference on some host architectures). Newer versions of clang warn about this. Avoid the bug by not using the "modify in place" byte swapping functions. There are a few places where the in-place swap function is used on something other than a packed struct field; we convert those anyway, for consistency. This patch was produced with the following spatch script: @@ expression E; @@ -be16_to_cpus(&E); +E = be16_to_cpu(E); @@ expression E; @@ -be32_to_cpus(&E); +E = be32_to_cpu(E); @@ expression E; @@ -be64_to_cpus(&E); +E = be64_to_cpu(E); @@ expression E; @@ -cpu_to_be16s(&E); +E = cpu_to_be16(E); @@ expression E; @@ -cpu_to_be32s(&E); +E = cpu_to_be32(E); @@ expression E; @@ -cpu_to_be64s(&E); +E = cpu_to_be64(E); Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Tested-by: John Snow <jsnow@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2-bitmap.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -XXX,XX +XXX,XX @@ static inline void bitmap_table_to_be(uint64_t *bitmap_table, size_t size) size_t i; for (i = 0; i < size; ++i) { - cpu_to_be64s(&bitmap_table[i]); + bitmap_table[i] = cpu_to_be64(bitmap_table[i]); } } @@ -XXX,XX +XXX,XX @@ static int bitmap_table_load(BlockDriverState *bs, Qcow2BitmapTable *tb, } for (i = 0; i < tb->size; ++i) { - be64_to_cpus(&table[i]); + table[i] = be64_to_cpu(table[i]); ret = check_table_entry(table[i], s->cluster_size); if (ret < 0) { goto fail; @@ -XXX,XX +XXX,XX @@ fail: static inline void bitmap_dir_entry_to_cpu(Qcow2BitmapDirEntry *entry) { - be64_to_cpus(&entry->bitmap_table_offset); - be32_to_cpus(&entry->bitmap_table_size); - be32_to_cpus(&entry->flags); - be16_to_cpus(&entry->name_size); - be32_to_cpus(&entry->extra_data_size); + entry->bitmap_table_offset = be64_to_cpu(entry->bitmap_table_offset); + entry->bitmap_table_size = be32_to_cpu(entry->bitmap_table_size); + entry->flags = be32_to_cpu(entry->flags); + entry->name_size = be16_to_cpu(entry->name_size); + entry->extra_data_size = be32_to_cpu(entry->extra_data_size); } static inline void bitmap_dir_entry_to_be(Qcow2BitmapDirEntry *entry) { - cpu_to_be64s(&entry->bitmap_table_offset); - cpu_to_be32s(&entry->bitmap_table_size); - cpu_to_be32s(&entry->flags); - cpu_to_be16s(&entry->name_size); - cpu_to_be32s(&entry->extra_data_size); + entry->bitmap_table_offset = cpu_to_be64(entry->bitmap_table_offset); + entry->bitmap_table_size = cpu_to_be32(entry->bitmap_table_size); + entry->flags = cpu_to_be32(entry->flags); + entry->name_size = cpu_to_be16(entry->name_size); + entry->extra_data_size = cpu_to_be32(entry->extra_data_size); } static inline int calc_dir_entry_size(size_t name_size, size_t extra_data_size) -- 2.19.1 From: Daniel P. Berrangé <berrange@redhat.com> The qcow2 block driver expects to see a valid sector size even when it has opened the crypto layer with QCRYPTO_BLOCK_OPEN_NO_IO. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- crypto/block-qcow.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c index XXXXXXX..XXXXXXX 100644 --- a/crypto/block-qcow.c +++ b/crypto/block-qcow.c @@ -XXX,XX +XXX,XX @@ qcrypto_block_qcow_open(QCryptoBlock *block, Error **errp) { if (flags & QCRYPTO_BLOCK_OPEN_NO_IO) { + block->sector_size = QCRYPTO_BLOCK_QCOW_SECTOR_SIZE; + block->payload_offset = 0; return 0; } else { if (!options->u.qcow.key_secret) { -- 2.19.1 From: Alberto Garcia <berto@igalia.com> This doesn't have any practical effect at the moment because the values of BDRV_SECTOR_SIZE, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE and QCRYPTO_BLOCK_QCOW_SECTOR_SIZE are all the same (512 bytes), but future encryption methods could have different requirements. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ static void qcow2_refresh_limits(BlockDriverState *bs, Error **errp) if (bs->encrypted) { /* Encryption works on a sector granularity */ - bs->bl.request_alignment = BDRV_SECTOR_SIZE; + bs->bl.request_alignment = qcrypto_block_get_sector_size(s->crypto); } bs->bl.pwrite_zeroes_alignment = s->cluster_size; bs->bl.pdiscard_alignment = s->cluster_size; -- 2.19.1 From: Li Qiang <liq3ea@163.com> Signed-off-by: Li Qiang <liq3ea@163.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/sysemu/block-backend.h | 6 +++--- block/block-backend.c | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index XXXXXXX..XXXXXXX 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -XXX,XX +XXX,XX @@ BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read, int error); void blk_error_action(BlockBackend *blk, BlockErrorAction action, bool is_read, int error); -int blk_is_read_only(BlockBackend *blk); -int blk_is_sg(BlockBackend *blk); -int blk_enable_write_cache(BlockBackend *blk); +bool blk_is_read_only(BlockBackend *blk); +bool blk_is_sg(BlockBackend *blk); +bool blk_enable_write_cache(BlockBackend *blk); void blk_set_enable_write_cache(BlockBackend *blk, bool wce); void blk_invalidate_cache(BlockBackend *blk, Error **errp); bool blk_is_inserted(BlockBackend *blk); diff --git a/block/block-backend.c b/block/block-backend.c index XXXXXXX..XXXXXXX 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -XXX,XX +XXX,XX @@ void blk_error_action(BlockBackend *blk, BlockErrorAction action, } } -int blk_is_read_only(BlockBackend *blk) +bool blk_is_read_only(BlockBackend *blk) { BlockDriverState *bs = blk_bs(blk); @@ -XXX,XX +XXX,XX @@ int blk_is_read_only(BlockBackend *blk) } } -int blk_is_sg(BlockBackend *blk) +bool blk_is_sg(BlockBackend *blk) { BlockDriverState *bs = blk_bs(blk); if (!bs) { - return 0; + return false; } return bdrv_is_sg(bs); } -int blk_enable_write_cache(BlockBackend *blk) +bool blk_enable_write_cache(BlockBackend *blk) { return blk->enable_write_cache; } -- 2.19.1 From: Cleber Rosa <crosa@redhat.com> While testing the Python 3 changes which touch the 083 test, I noticed that it would fail with qcow2. Expanding the testing, I noticed it had nothing to do with the Python 3 changes, and in fact, it would not pass on anything but raw: raw: pass bochs: not generic cloop: not generic parallels: fail qcow: fail qcow2: fail qed: fail vdi: fail vhdx: fail vmdk: fail vpc: fail luks: fail The errors are a mixture I/O and "image not in xxx format", such as: === Check disconnect before data === Unexpected end-of-file before all bytes were read -read failed: Input/output error +can't open device nbd+tcp://127.0.0.1:PORT/foo: Could not open 'nbd://127.0.0.1:PORT/foo': Input/output error === Check disconnect after data === -read 512/512 bytes at offset 0 -512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +can't open device nbd+tcp://127.0.0.1:PORT/foo: Image not in qcow format I'm not aware if there's a quick fix, so, for the time being, it looks like the honest approach is to make the test known to work on raw only. Signed-off-by: Cleber Rosa <crosa@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/083 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/083 +++ b/tests/qemu-iotests/083 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.rc . ./common.filter -_supported_fmt generic +_supported_fmt raw _supported_proto nbd _supported_os Linux -- 2.19.1 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> --- block/vpc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/block/vpc.c b/block/vpc.c index XXXXXXX..XXXXXXX 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -XXX,XX +XXX,XX @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, } qemu_co_mutex_init(&s->lock); + qemu_opts_del(opts); return 0; fail: + qemu_opts_del(opts); qemu_vfree(s->pagetable); #ifdef CACHE g_free(s->pageentry_u8); -- 2.19.1 From: Peter Maydell <peter.maydell@linaro.org> Taking the address of a field in a packed struct is a bad idea, because it might not be actually aligned enough for that pointer type (and thus cause a crash on dereference on some host architectures). Newer versions of clang warn about this. Avoid the bug by not using the "modify in place" byte swapping functions. There are a few places where the in-place swap function is used on something other than a packed struct field; we convert those anyway, for consistency. Patch produced with scripts/coccinelle/inplace-byteswaps.cocci. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vhdx.h | 12 ++--- block/vhdx-endian.c | 118 ++++++++++++++++++++++---------------------- block/vhdx-log.c | 4 +- block/vhdx.c | 18 +++---- 4 files changed, 76 insertions(+), 76 deletions(-) diff --git a/block/vhdx.h b/block/vhdx.h index XXXXXXX..XXXXXXX 100644 --- a/block/vhdx.h +++ b/block/vhdx.h @@ -XXX,XX +XXX,XX @@ int vhdx_log_write_and_flush(BlockDriverState *bs, BDRVVHDXState *s, static inline void leguid_to_cpus(MSGUID *guid) { - le32_to_cpus(&guid->data1); - le16_to_cpus(&guid->data2); - le16_to_cpus(&guid->data3); + guid->data1 = le32_to_cpu(guid->data1); + guid->data2 = le16_to_cpu(guid->data2); + guid->data3 = le16_to_cpu(guid->data3); } static inline void cpu_to_leguids(MSGUID *guid) { - cpu_to_le32s(&guid->data1); - cpu_to_le16s(&guid->data2); - cpu_to_le16s(&guid->data3); + guid->data1 = cpu_to_le32(guid->data1); + guid->data2 = cpu_to_le16(guid->data2); + guid->data3 = cpu_to_le16(guid->data3); } void vhdx_header_le_import(VHDXHeader *h); diff --git a/block/vhdx-endian.c b/block/vhdx-endian.c index XXXXXXX..XXXXXXX 100644 --- a/block/vhdx-endian.c +++ b/block/vhdx-endian.c @@ -XXX,XX +XXX,XX @@ void vhdx_header_le_import(VHDXHeader *h) { assert(h != NULL); - le32_to_cpus(&h->signature); - le32_to_cpus(&h->checksum); - le64_to_cpus(&h->sequence_number); + h->signature = le32_to_cpu(h->signature); + h->checksum = le32_to_cpu(h->checksum); + h->sequence_number = le64_to_cpu(h->sequence_number); leguid_to_cpus(&h->file_write_guid); leguid_to_cpus(&h->data_write_guid); leguid_to_cpus(&h->log_guid); - le16_to_cpus(&h->log_version); - le16_to_cpus(&h->version); - le32_to_cpus(&h->log_length); - le64_to_cpus(&h->log_offset); + h->log_version = le16_to_cpu(h->log_version); + h->version = le16_to_cpu(h->version); + h->log_length = le32_to_cpu(h->log_length); + h->log_offset = le64_to_cpu(h->log_offset); } void vhdx_header_le_export(VHDXHeader *orig_h, VHDXHeader *new_h) @@ -XXX,XX +XXX,XX @@ void vhdx_log_desc_le_import(VHDXLogDescriptor *d) { assert(d != NULL); - le32_to_cpus(&d->signature); - le64_to_cpus(&d->file_offset); - le64_to_cpus(&d->sequence_number); + d->signature = le32_to_cpu(d->signature); + d->file_offset = le64_to_cpu(d->file_offset); + d->sequence_number = le64_to_cpu(d->sequence_number); } void vhdx_log_desc_le_export(VHDXLogDescriptor *d) { assert(d != NULL); - cpu_to_le32s(&d->signature); - cpu_to_le32s(&d->trailing_bytes); - cpu_to_le64s(&d->leading_bytes); - cpu_to_le64s(&d->file_offset); - cpu_to_le64s(&d->sequence_number); + d->signature = cpu_to_le32(d->signature); + d->trailing_bytes = cpu_to_le32(d->trailing_bytes); + d->leading_bytes = cpu_to_le64(d->leading_bytes); + d->file_offset = cpu_to_le64(d->file_offset); + d->sequence_number = cpu_to_le64(d->sequence_number); } void vhdx_log_data_le_import(VHDXLogDataSector *d) { assert(d != NULL); - le32_to_cpus(&d->data_signature); - le32_to_cpus(&d->sequence_high); - le32_to_cpus(&d->sequence_low); + d->data_signature = le32_to_cpu(d->data_signature); + d->sequence_high = le32_to_cpu(d->sequence_high); + d->sequence_low = le32_to_cpu(d->sequence_low); } void vhdx_log_data_le_export(VHDXLogDataSector *d) { assert(d != NULL); - cpu_to_le32s(&d->data_signature); - cpu_to_le32s(&d->sequence_high); - cpu_to_le32s(&d->sequence_low); + d->data_signature = cpu_to_le32(d->data_signature); + d->sequence_high = cpu_to_le32(d->sequence_high); + d->sequence_low = cpu_to_le32(d->sequence_low); } void vhdx_log_entry_hdr_le_import(VHDXLogEntryHeader *hdr) { assert(hdr != NULL); - le32_to_cpus(&hdr->signature); - le32_to_cpus(&hdr->checksum); - le32_to_cpus(&hdr->entry_length); - le32_to_cpus(&hdr->tail); - le64_to_cpus(&hdr->sequence_number); - le32_to_cpus(&hdr->descriptor_count); + hdr->signature = le32_to_cpu(hdr->signature); + hdr->checksum = le32_to_cpu(hdr->checksum); + hdr->entry_length = le32_to_cpu(hdr->entry_length); + hdr->tail = le32_to_cpu(hdr->tail); + hdr->sequence_number = le64_to_cpu(hdr->sequence_number); + hdr->descriptor_count = le32_to_cpu(hdr->descriptor_count); leguid_to_cpus(&hdr->log_guid); - le64_to_cpus(&hdr->flushed_file_offset); - le64_to_cpus(&hdr->last_file_offset); + hdr->flushed_file_offset = le64_to_cpu(hdr->flushed_file_offset); + hdr->last_file_offset = le64_to_cpu(hdr->last_file_offset); } void vhdx_log_entry_hdr_le_export(VHDXLogEntryHeader *hdr) { assert(hdr != NULL); - cpu_to_le32s(&hdr->signature); - cpu_to_le32s(&hdr->checksum); - cpu_to_le32s(&hdr->entry_length); - cpu_to_le32s(&hdr->tail); - cpu_to_le64s(&hdr->sequence_number); - cpu_to_le32s(&hdr->descriptor_count); + hdr->signature = cpu_to_le32(hdr->signature); + hdr->checksum = cpu_to_le32(hdr->checksum); + hdr->entry_length = cpu_to_le32(hdr->entry_length); + hdr->tail = cpu_to_le32(hdr->tail); + hdr->sequence_number = cpu_to_le64(hdr->sequence_number); + hdr->descriptor_count = cpu_to_le32(hdr->descriptor_count); cpu_to_leguids(&hdr->log_guid); - cpu_to_le64s(&hdr->flushed_file_offset); - cpu_to_le64s(&hdr->last_file_offset); + hdr->flushed_file_offset = cpu_to_le64(hdr->flushed_file_offset); + hdr->last_file_offset = cpu_to_le64(hdr->last_file_offset); } @@ -XXX,XX +XXX,XX @@ void vhdx_region_header_le_import(VHDXRegionTableHeader *hdr) { assert(hdr != NULL); - le32_to_cpus(&hdr->signature); - le32_to_cpus(&hdr->checksum); - le32_to_cpus(&hdr->entry_count); + hdr->signature = le32_to_cpu(hdr->signature); + hdr->checksum = le32_to_cpu(hdr->checksum); + hdr->entry_count = le32_to_cpu(hdr->entry_count); } void vhdx_region_header_le_export(VHDXRegionTableHeader *hdr) { assert(hdr != NULL); - cpu_to_le32s(&hdr->signature); - cpu_to_le32s(&hdr->checksum); - cpu_to_le32s(&hdr->entry_count); + hdr->signature = cpu_to_le32(hdr->signature); + hdr->checksum = cpu_to_le32(hdr->checksum); + hdr->entry_count = cpu_to_le32(hdr->entry_count); } void vhdx_region_entry_le_import(VHDXRegionTableEntry *e) @@ -XXX,XX +XXX,XX @@ void vhdx_region_entry_le_import(VHDXRegionTableEntry *e) assert(e != NULL); leguid_to_cpus(&e->guid); - le64_to_cpus(&e->file_offset); - le32_to_cpus(&e->length); - le32_to_cpus(&e->data_bits); + e->file_offset = le64_to_cpu(e->file_offset); + e->length = le32_to_cpu(e->length); + e->data_bits = le32_to_cpu(e->data_bits); } void vhdx_region_entry_le_export(VHDXRegionTableEntry *e) @@ -XXX,XX +XXX,XX @@ void vhdx_region_entry_le_export(VHDXRegionTableEntry *e) assert(e != NULL); cpu_to_leguids(&e->guid); - cpu_to_le64s(&e->file_offset); - cpu_to_le32s(&e->length); - cpu_to_le32s(&e->data_bits); + e->file_offset = cpu_to_le64(e->file_offset); + e->length = cpu_to_le32(e->length); + e->data_bits = cpu_to_le32(e->data_bits); } @@ -XXX,XX +XXX,XX @@ void vhdx_metadata_header_le_import(VHDXMetadataTableHeader *hdr) { assert(hdr != NULL); - le64_to_cpus(&hdr->signature); - le16_to_cpus(&hdr->entry_count); + hdr->signature = le64_to_cpu(hdr->signature); + hdr->entry_count = le16_to_cpu(hdr->entry_count); } void vhdx_metadata_header_le_export(VHDXMetadataTableHeader *hdr) { assert(hdr != NULL); - cpu_to_le64s(&hdr->signature); - cpu_to_le16s(&hdr->entry_count); + hdr->signature = cpu_to_le64(hdr->signature); + hdr->entry_count = cpu_to_le16(hdr->entry_count); } void vhdx_metadata_entry_le_import(VHDXMetadataTableEntry *e) @@ -XXX,XX +XXX,XX @@ void vhdx_metadata_entry_le_import(VHDXMetadataTableEntry *e) assert(e != NULL); leguid_to_cpus(&e->item_id); - le32_to_cpus(&e->offset); - le32_to_cpus(&e->length); - le32_to_cpus(&e->data_bits); + e->offset = le32_to_cpu(e->offset); + e->length = le32_to_cpu(e->length); + e->data_bits = le32_to_cpu(e->data_bits); } void vhdx_metadata_entry_le_export(VHDXMetadataTableEntry *e) { assert(e != NULL); cpu_to_leguids(&e->item_id); - cpu_to_le32s(&e->offset); - cpu_to_le32s(&e->length); - cpu_to_le32s(&e->data_bits); + e->offset = cpu_to_le32(e->offset); + e->length = cpu_to_le32(e->length); + e->data_bits = cpu_to_le32(e->data_bits); } diff --git a/block/vhdx-log.c b/block/vhdx-log.c index XXXXXXX..XXXXXXX 100644 --- a/block/vhdx-log.c +++ b/block/vhdx-log.c @@ -XXX,XX +XXX,XX @@ static void vhdx_log_raw_to_le_sector(VHDXLogDescriptor *desc, /* 8 + 4084 + 4 = 4096, 1 log sector */ memcpy(&desc->leading_bytes, data, 8); data += 8; - cpu_to_le64s(&desc->leading_bytes); + desc->leading_bytes = cpu_to_le64(desc->leading_bytes); memcpy(sector->data, data, 4084); data += 4084; memcpy(&desc->trailing_bytes, data, 4); - cpu_to_le32s(&desc->trailing_bytes); + desc->trailing_bytes = cpu_to_le32(desc->trailing_bytes); data += 4; sector->sequence_high = (uint32_t) (seq >> 32); diff --git a/block/vhdx.c b/block/vhdx.c index XXXXXXX..XXXXXXX 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -XXX,XX +XXX,XX @@ uint32_t vhdx_update_checksum(uint8_t *buf, size_t size, int crc_offset) memset(buf + crc_offset, 0, sizeof(crc)); crc = crc32c(0xffffffff, buf, size); - cpu_to_le32s(&crc); + crc = cpu_to_le32(crc); memcpy(buf + crc_offset, &crc, sizeof(crc)); return crc; @@ -XXX,XX +XXX,XX @@ static int vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s) goto exit; } - le32_to_cpus(&s->params.block_size); - le32_to_cpus(&s->params.data_bits); + s->params.block_size = le32_to_cpu(s->params.block_size); + s->params.data_bits = le32_to_cpu(s->params.data_bits); /* We now have the file parameters, so we can tell if this is a @@ -XXX,XX +XXX,XX @@ static int vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s) goto exit; } - le64_to_cpus(&s->virtual_disk_size); - le32_to_cpus(&s->logical_sector_size); - le32_to_cpus(&s->physical_sector_size); + s->virtual_disk_size = le64_to_cpu(s->virtual_disk_size); + s->logical_sector_size = le32_to_cpu(s->logical_sector_size); + s->physical_sector_size = le32_to_cpu(s->physical_sector_size); if (s->params.block_size < VHDX_BLOCK_SIZE_MIN || s->params.block_size > VHDX_BLOCK_SIZE_MAX) { @@ -XXX,XX +XXX,XX @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, /* endian convert, and verify populated BAT field file offsets against * region table and log entries */ for (i = 0; i < s->bat_entries; i++) { - le64_to_cpus(&s->bat[i]); + s->bat[i] = le64_to_cpu(s->bat[i]); if (payblocks--) { /* payload bat entries */ if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) == @@ -XXX,XX +XXX,XX @@ static int vhdx_create_new_metadata(BlockBackend *blk, mt_file_params->block_size = cpu_to_le32(block_size); if (type == VHDX_TYPE_FIXED) { mt_file_params->data_bits |= VHDX_PARAMS_LEAVE_BLOCKS_ALLOCED; - cpu_to_le32s(&mt_file_params->data_bits); + mt_file_params->data_bits = cpu_to_le32(mt_file_params->data_bits); } vhdx_guid_generate(&mt_page83->page_83_data); @@ -XXX,XX +XXX,XX @@ static int vhdx_create_bat(BlockBackend *blk, BDRVVHDXState *s, sinfo.file_offset = ROUND_UP(sinfo.file_offset, MiB); vhdx_update_bat_table_entry(blk_bs(blk), s, &sinfo, &unused, &unused, block_state); - cpu_to_le64s(&s->bat[sinfo.bat_idx]); + s->bat[sinfo.bat_idx] = cpu_to_le64(s->bat[sinfo.bat_idx]); sector_num += s->sectors_per_block; } ret = blk_pwrite(blk, file_offset, s->bat, length, 0); -- 2.19.1 From: Peter Maydell <peter.maydell@linaro.org> Taking the address of a field in a packed struct is a bad idea, because it might not be actually aligned enough for that pointer type (and thus cause a crash on dereference on some host architectures). Newer versions of clang warn about this. Avoid the bug by not using the "modify in place" byte swapping functions. There are a few places where the in-place swap function is used on something other than a packed struct field; we convert those anyway, for consistency. Patch produced with scripts/coccinelle/inplace-byteswaps.cocci. There are other places where we take the address of a packed member in this file for other purposes than passing it to a byteswap function (all the calls to qemu_uuid_*()); we leave those for now. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vdi.c | 64 ++++++++++++++++++++++++++--------------------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/block/vdi.c b/block/vdi.c index XXXXXXX..XXXXXXX 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -XXX,XX +XXX,XX @@ typedef struct { static void vdi_header_to_cpu(VdiHeader *header) { - le32_to_cpus(&header->signature); - le32_to_cpus(&header->version); - le32_to_cpus(&header->header_size); - le32_to_cpus(&header->image_type); - le32_to_cpus(&header->image_flags); - le32_to_cpus(&header->offset_bmap); - le32_to_cpus(&header->offset_data); - le32_to_cpus(&header->cylinders); - le32_to_cpus(&header->heads); - le32_to_cpus(&header->sectors); - le32_to_cpus(&header->sector_size); - le64_to_cpus(&header->disk_size); - le32_to_cpus(&header->block_size); - le32_to_cpus(&header->block_extra); - le32_to_cpus(&header->blocks_in_image); - le32_to_cpus(&header->blocks_allocated); + header->signature = le32_to_cpu(header->signature); + header->version = le32_to_cpu(header->version); + header->header_size = le32_to_cpu(header->header_size); + header->image_type = le32_to_cpu(header->image_type); + header->image_flags = le32_to_cpu(header->image_flags); + header->offset_bmap = le32_to_cpu(header->offset_bmap); + header->offset_data = le32_to_cpu(header->offset_data); + header->cylinders = le32_to_cpu(header->cylinders); + header->heads = le32_to_cpu(header->heads); + header->sectors = le32_to_cpu(header->sectors); + header->sector_size = le32_to_cpu(header->sector_size); + header->disk_size = le64_to_cpu(header->disk_size); + header->block_size = le32_to_cpu(header->block_size); + header->block_extra = le32_to_cpu(header->block_extra); + header->blocks_in_image = le32_to_cpu(header->blocks_in_image); + header->blocks_allocated = le32_to_cpu(header->blocks_allocated); qemu_uuid_bswap(&header->uuid_image); qemu_uuid_bswap(&header->uuid_last_snap); qemu_uuid_bswap(&header->uuid_link); @@ -XXX,XX +XXX,XX @@ static void vdi_header_to_cpu(VdiHeader *header) static void vdi_header_to_le(VdiHeader *header) { - cpu_to_le32s(&header->signature); - cpu_to_le32s(&header->version); - cpu_to_le32s(&header->header_size); - cpu_to_le32s(&header->image_type); - cpu_to_le32s(&header->image_flags); - cpu_to_le32s(&header->offset_bmap); - cpu_to_le32s(&header->offset_data); - cpu_to_le32s(&header->cylinders); - cpu_to_le32s(&header->heads); - cpu_to_le32s(&header->sectors); - cpu_to_le32s(&header->sector_size); - cpu_to_le64s(&header->disk_size); - cpu_to_le32s(&header->block_size); - cpu_to_le32s(&header->block_extra); - cpu_to_le32s(&header->blocks_in_image); - cpu_to_le32s(&header->blocks_allocated); + header->signature = cpu_to_le32(header->signature); + header->version = cpu_to_le32(header->version); + header->header_size = cpu_to_le32(header->header_size); + header->image_type = cpu_to_le32(header->image_type); + header->image_flags = cpu_to_le32(header->image_flags); + header->offset_bmap = cpu_to_le32(header->offset_bmap); + header->offset_data = cpu_to_le32(header->offset_data); + header->cylinders = cpu_to_le32(header->cylinders); + header->heads = cpu_to_le32(header->heads); + header->sectors = cpu_to_le32(header->sectors); + header->sector_size = cpu_to_le32(header->sector_size); + header->disk_size = cpu_to_le64(header->disk_size); + header->block_size = cpu_to_le32(header->block_size); + header->block_extra = cpu_to_le32(header->block_extra); + header->blocks_in_image = cpu_to_le32(header->blocks_in_image); + header->blocks_allocated = cpu_to_le32(header->blocks_allocated); qemu_uuid_bswap(&header->uuid_image); qemu_uuid_bswap(&header->uuid_last_snap); qemu_uuid_bswap(&header->uuid_link); -- 2.19.1 From: Alberto Garcia <berto@igalia.com> This is a static function with only one caller, so there's no need to keep it. Inlining the code in quorum_compare() makes it much simpler. Signed-off-by: Alberto Garcia <berto@igalia.com> Reported-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/quorum.c | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/block/quorum.c b/block/quorum.c index XXXXXXX..XXXXXXX 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -XXX,XX +XXX,XX @@ static bool quorum_iovec_compare(QEMUIOVector *a, QEMUIOVector *b) return true; } -static void GCC_FMT_ATTR(2, 3) quorum_err(QuorumAIOCB *acb, - const char *fmt, ...) -{ - va_list ap; - - va_start(ap, fmt); - fprintf(stderr, "quorum: offset=%" PRIu64 " bytes=%" PRIu64 " ", - acb->offset, acb->bytes); - vfprintf(stderr, fmt, ap); - fprintf(stderr, "\n"); - va_end(ap); - exit(1); -} - -static bool quorum_compare(QuorumAIOCB *acb, - QEMUIOVector *a, - QEMUIOVector *b) +static bool quorum_compare(QuorumAIOCB *acb, QEMUIOVector *a, QEMUIOVector *b) { BDRVQuorumState *s = acb->bs->opaque; ssize_t offset; @@ -XXX,XX +XXX,XX @@ static bool quorum_compare(QuorumAIOCB *acb, if (s->is_blkverify) { offset = qemu_iovec_compare(a, b); if (offset != -1) { - quorum_err(acb, "contents mismatch at offset %" PRIu64, - acb->offset + offset); + fprintf(stderr, "quorum: offset=%" PRIu64 " bytes=%" PRIu64 + " contents mismatch at offset %" PRIu64 "\n", + acb->offset, acb->bytes, acb->offset + offset); + exit(1); } return true; } -- 2.19.1 From: Alberto Garcia <berto@igalia.com> The blkverify mode of Quorum can only be enabled if the number of children is exactly two and the value of vote-threshold is also two. If the user tries to enable it but the other settings are incorrect then QEMU simply prints an error message to stderr and carries on disabling the blkverify setting. This patch makes quorum_open() fail and return an error in this case. Signed-off-by: Alberto Garcia <berto@igalia.com> Reported-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/quorum.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/block/quorum.c b/block/quorum.c index XXXXXXX..XXXXXXX 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -XXX,XX +XXX,XX @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags, s->read_pattern = ret; if (s->read_pattern == QUORUM_READ_PATTERN_QUORUM) { - /* is the driver in blkverify mode */ - if (qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false) && - s->num_children == 2 && s->threshold == 2) { - s->is_blkverify = true; - } else if (qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false)) { - fprintf(stderr, "blkverify mode is set by setting blkverify=on " - "and using two files with vote_threshold=2\n"); + s->is_blkverify = qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false); + if (s->is_blkverify && (s->num_children != 2 || s->threshold != 2)) { + error_setg(&local_err, "blkverify=on can only be set if there are " + "exactly two files and vote-threshold is 2"); + ret = -EINVAL; + goto exit; } s->rewrite_corrupted = qemu_opt_get_bool(opts, QUORUM_OPT_REWRITE, -- 2.19.1 From: Alberto Garcia <berto@igalia.com> Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/081 | 30 ++++++++++++++++++++++++++++++ tests/qemu-iotests/081.out | 16 ++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/081 +++ b/tests/qemu-iotests/081 @@ -XXX,XX +XXX,XX @@ echo "== checking that quorum is broken ==" $QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io +echo +echo "== checking the blkverify mode with broken content ==" + +quorum="driver=raw,file.driver=quorum,file.vote-threshold=2,file.blkverify=on" +quorum="$quorum,file.children.0.file.filename=$TEST_DIR/1.raw" +quorum="$quorum,file.children.1.file.filename=$TEST_DIR/2.raw" +quorum="$quorum,file.children.0.driver=raw" +quorum="$quorum,file.children.1.driver=raw" + +$QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io + +echo +echo "== writing the same data to both files ==" + +$QEMU_IO -c "write -P 0x32 0 $size" "$TEST_DIR/1.raw" | _filter_qemu_io +$QEMU_IO -c "write -P 0x32 0 $size" "$TEST_DIR/2.raw" | _filter_qemu_io + +echo +echo "== checking the blkverify mode with valid content ==" + +$QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io + +echo +echo "== checking the blkverify mode with invalid settings ==" + +quorum="$quorum,file.children.2.file.filename=$TEST_DIR/3.raw" +quorum="$quorum,file.children.2.driver=raw" + +$QEMU_IO -c "open -o $quorum" | _filter_qemu_io + # success, all done echo "*** done" rm -f $seq.full diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/081.out +++ b/tests/qemu-iotests/081.out @@ -XXX,XX +XXX,XX @@ wrote 10485760/10485760 bytes at offset 0 == checking that quorum is broken == read failed: Input/output error + +== checking the blkverify mode with broken content == +quorum: offset=0 bytes=10485760 contents mismatch at offset 0 + +== writing the same data to both files == +wrote 10485760/10485760 bytes at offset 0 +10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 10485760/10485760 bytes at offset 0 +10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== checking the blkverify mode with valid content == +read 10485760/10485760 bytes at offset 0 +10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== checking the blkverify mode with invalid settings == +can't open: blkverify=on can only be set if there are exactly two files and vote-threshold is 2 *** done -- 2.19.1 From: Alberto Garcia <berto@igalia.com> The blkverify mode of Quorum only works when the number of children is exactly two, so any attempt to add a new one must return an error. quorum_del_child() on the other hand doesn't need any additional check because decreasing the number of children would make it go under the vote threshold. Signed-off-by: Alberto Garcia <berto@igalia.com> Reported-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/quorum.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/block/quorum.c b/block/quorum.c index XXXXXXX..XXXXXXX 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -XXX,XX +XXX,XX @@ static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs, char indexstr[32]; int ret; + if (s->is_blkverify) { + error_setg(errp, "Cannot add a child to a quorum in blkverify mode"); + return; + } + assert(s->num_children <= INT_MAX / sizeof(BdrvChild *)); if (s->num_children == INT_MAX / sizeof(BdrvChild *) || s->next_child_index == UINT_MAX) { @@ -XXX,XX +XXX,XX @@ static void quorum_del_child(BlockDriverState *bs, BdrvChild *child, return; } + /* We know now that num_children > threshold, so blkverify must be false */ + assert(!s->is_blkverify); + bdrv_drained_begin(bs); /* We can safely remove this child now */ -- 2.19.1 From: Alberto Garcia <berto@igalia.com> This patch tests that you can add and remove drives from a Quorum using the x-blockdev-change command. Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/081 | 86 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/081.out | 54 ++++++++++++++++++++++++ 2 files changed, 140 insertions(+) diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/081 +++ b/tests/qemu-iotests/081 @@ -XXX,XX +XXX,XX @@ quorum="$quorum,file.children.2.driver=raw" $QEMU_IO -c "open -o $quorum" | _filter_qemu_io +echo +echo "== dynamically adding a child to a quorum ==" + +for verify in false true; do + run_qemu <<EOF + { "execute": "qmp_capabilities" } + { "execute": "blockdev-add", + "arguments": { + "driver": "quorum", + "node-name": "drive0-quorum", + "vote-threshold": 2, + "blkverify": ${verify}, + "children": [ + { + "driver": "$IMGFMT", + "file": { + "driver": "file", + "filename": "$TEST_DIR/1.raw" + } + }, + { + "driver": "$IMGFMT", + "file": { + "driver": "file", + "filename": "$TEST_DIR/2.raw" + } + } + ] + } + } + { "execute": "blockdev-add", + "arguments": { + "node-name": "drive3", + "driver": "$IMGFMT", + "file": { + "driver": "file", + "filename": "$TEST_DIR/2.raw" + } + } + } + { "execute": "x-blockdev-change", + "arguments": { "parent": "drive0-quorum", + "node": "drive3" } } + { "execute": "quit" } +EOF +done + +echo +echo "== dynamically removing a child from a quorum ==" + +for verify in false true; do + for vote_threshold in 1 2; do + run_qemu <<EOF + { "execute": "qmp_capabilities" } + { "execute": "blockdev-add", + "arguments": { + "driver": "quorum", + "node-name": "drive0-quorum", + "vote-threshold": ${vote_threshold}, + "blkverify": ${verify}, + "children": [ + { + "driver": "$IMGFMT", + "file": { + "driver": "file", + "filename": "$TEST_DIR/1.raw" + } + }, + { + "driver": "$IMGFMT", + "file": { + "driver": "file", + "filename": "$TEST_DIR/2.raw" + } + } + ] + } + } + { "execute": "x-blockdev-change", + "arguments": { "parent": "drive0-quorum", + "child": "children.1" } } + { "execute": "quit" } +EOF + done +done + # success, all done echo "*** done" rm -f $seq.full diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/081.out +++ b/tests/qemu-iotests/081.out @@ -XXX,XX +XXX,XX @@ read 10485760/10485760 bytes at offset 0 == checking the blkverify mode with invalid settings == can't open: blkverify=on can only be set if there are exactly two files and vote-threshold is 2 + +== dynamically adding a child to a quorum == +Testing: +QMP_VERSION +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} + +Testing: +QMP_VERSION +{"return": {}} +{"return": {}} +{"return": {}} +{"error": {"class": "GenericError", "desc": "Cannot add a child to a quorum in blkverify mode"}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} + + +== dynamically removing a child from a quorum == +Testing: +QMP_VERSION +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} + +Testing: +QMP_VERSION +{"return": {}} +{"return": {}} +{"error": {"class": "GenericError", "desc": "The number of children cannot be lower than the vote threshold 2"}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} + +Testing: +QMP_VERSION +{"return": {}} +{"error": {"class": "GenericError", "desc": "blkverify=on can only be set if there are exactly two files and vote-threshold is 2"}} +{"error": {"class": "GenericError", "desc": "Cannot find device=drive0-quorum nor node_name=drive0-quorum"}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} + +Testing: +QMP_VERSION +{"return": {}} +{"return": {}} +{"error": {"class": "GenericError", "desc": "The number of children cannot be lower than the vote threshold 2"}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} + *** done -- 2.19.1 To fully change the read-only state of a node, we must not only change bs->read_only, but also update bs->open_flags. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> --- block.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp) } bs->read_only = read_only; + + if (read_only) { + bs->open_flags &= ~BDRV_O_RDWR; + } else { + bs->open_flags |= BDRV_O_RDWR; + } + return 0; } -- 2.19.1 If a management application builds the block graph node by node, the protocol layer doesn't inherit its read-only option from the format layer any more, so it must be set explicitly. Backing files should work on read-only storage, but at the same time, a block job like commit should be able to reopen them read-write if they are on read-write storage. However, without option inheritance, reopen only changes the read-only option for the root node (typically the format layer), but not the protocol layer, so reopening fails (the format layer wants to get write permissions, but the protocol layer is still read-only). A simple workaround for the problem in the management tool would be to open the protocol layer always read-write and to make only the format layer read-only for backing files. However, sometimes the file is actually stored on read-only storage and we don't know whether the image can be opened read-write (for example, for NBD it depends on the server we're trying to connect to). This adds an option that makes QEMU try to open the image read-write, but allows it to degrade to a read-only mode without returning an error. The documentation for this option is consciously phrased in a way that allows QEMU to switch to a better model eventually: Instead of trying when the image is first opened, making the read-only flag dynamic and changing it automatically whenever the first BLK_PERM_WRITE user is attached or the last one is detached would be much more useful behaviour. Unfortunately, this more useful behaviour is also a lot harder to implement, and libvirt needs a solution now before it can switch to -blockdev, so let's start with this easier approach for now. Instead of adding a new auto-read-only option, turning the existing read-only into an enum (with a bool alternate for compatibility) was considered, but it complicated the implementation to the point that it didn't seem to be worth it. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- qapi/block-core.json | 7 +++++++ include/block/block.h | 2 ++ block.c | 17 +++++++++++++++++ block/vvfat.c | 1 + blockdev.c | 2 +- 5 files changed, 28 insertions(+), 1 deletion(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index XXXXXXX..XXXXXXX 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -XXX,XX +XXX,XX @@ # either generally or in certain configurations. In this case, # the default value does not work and the option must be # specified explicitly. +# @auto-read-only: if true and @read-only is false, QEMU may automatically +# decide not to open the image read-write as requested, but +# fall back to read-only instead (and switch between the modes +# later), e.g. depending on whether the image file is writable +# or whether a writing user is attached to the node +# (default: false, since 3.1) # @detect-zeroes: detect and optimize zero writes (Since 2.1) # (default: off) # @force-share: force share all permission on added nodes. @@ -XXX,XX +XXX,XX @@ '*discard': 'BlockdevDiscardOptions', '*cache': 'BlockdevCacheOptions', '*read-only': 'bool', + '*auto-read-only': 'bool', '*force-share': 'bool', '*detect-zeroes': 'BlockdevDetectZeroesOptions' }, 'discriminator': 'driver', diff --git a/include/block/block.h b/include/block/block.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -XXX,XX +XXX,XX @@ typedef struct HDGeometry { select an appropriate protocol driver, ignoring the format layer */ #define BDRV_O_NO_IO 0x10000 /* don't initialize for I/O */ +#define BDRV_O_AUTO_RDONLY 0x20000 /* degrade to read-only if opening read-write fails */ #define BDRV_O_CACHE_MASK (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH) @@ -XXX,XX +XXX,XX @@ typedef struct HDGeometry { #define BDRV_OPT_CACHE_DIRECT "cache.direct" #define BDRV_OPT_CACHE_NO_FLUSH "cache.no-flush" #define BDRV_OPT_READ_ONLY "read-only" +#define BDRV_OPT_AUTO_READ_ONLY "auto-read-only" #define BDRV_OPT_DISCARD "discard" #define BDRV_OPT_FORCE_SHARE "force-share" diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ static void bdrv_inherited_options(int *child_flags, QDict *child_options, /* Inherit the read-only option from the parent if it's not set */ qdict_copy_default(child_options, parent_options, BDRV_OPT_READ_ONLY); + qdict_copy_default(child_options, parent_options, BDRV_OPT_AUTO_READ_ONLY); /* Our block drivers take care to send flushes and respect unmap policy, * so we can default to enable both on lower layers regardless of the @@ -XXX,XX +XXX,XX @@ static void bdrv_backing_options(int *child_flags, QDict *child_options, /* backing files always opened read-only */ qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "on"); + qdict_set_default_str(child_options, BDRV_OPT_AUTO_READ_ONLY, "off"); flags &= ~BDRV_O_COPY_ON_READ; /* snapshot=on is handled on the top layer */ @@ -XXX,XX +XXX,XX @@ static void update_flags_from_options(int *flags, QemuOpts *opts) *flags |= BDRV_O_RDWR; } + assert(qemu_opt_find(opts, BDRV_OPT_AUTO_READ_ONLY)); + if (qemu_opt_get_bool_del(opts, BDRV_OPT_AUTO_READ_ONLY, false)) { + *flags |= BDRV_O_AUTO_RDONLY; + } } static void update_options_from_flags(QDict *options, int flags) @@ -XXX,XX +XXX,XX @@ static void update_options_from_flags(QDict *options, int flags) if (!qdict_haskey(options, BDRV_OPT_READ_ONLY)) { qdict_put_bool(options, BDRV_OPT_READ_ONLY, !(flags & BDRV_O_RDWR)); } + if (!qdict_haskey(options, BDRV_OPT_AUTO_READ_ONLY)) { + qdict_put_bool(options, BDRV_OPT_AUTO_READ_ONLY, + flags & BDRV_O_AUTO_RDONLY); + } } static void bdrv_assign_node_name(BlockDriverState *bs, @@ -XXX,XX +XXX,XX @@ QemuOptsList bdrv_runtime_opts = { .type = QEMU_OPT_BOOL, .help = "Node is opened in read-only mode", }, + { + .name = BDRV_OPT_AUTO_READ_ONLY, + .type = QEMU_OPT_BOOL, + .help = "Node can become read-only if opening read-write fails", + }, { .name = "detect-zeroes", .type = QEMU_OPT_STRING, @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp) qdict_set_default_str(qdict, BDRV_OPT_CACHE_DIRECT, "off"); qdict_set_default_str(qdict, BDRV_OPT_CACHE_NO_FLUSH, "off"); qdict_set_default_str(qdict, BDRV_OPT_READ_ONLY, "off"); + qdict_set_default_str(qdict, BDRV_OPT_AUTO_READ_ONLY, "off"); + } bs = bdrv_open_inherit(NULL, reference, qdict, 0, NULL, NULL, errp); diff --git a/block/vvfat.c b/block/vvfat.c index XXXXXXX..XXXXXXX 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -XXX,XX +XXX,XX @@ static void vvfat_qcow_options(int *child_flags, QDict *child_options, int parent_flags, QDict *parent_options) { qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "off"); + qdict_set_default_str(child_options, BDRV_OPT_AUTO_READ_ONLY, "off"); qdict_set_default_str(child_options, BDRV_OPT_CACHE_NO_FLUSH, "on"); } diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ void qmp_blockdev_change_medium(bool has_device, const char *device, bdrv_flags = blk_get_open_flags_from_root_state(blk); bdrv_flags &= ~(BDRV_O_TEMPORARY | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | - BDRV_O_PROTOCOL); + BDRV_O_PROTOCOL | BDRV_O_AUTO_RDONLY); if (!has_read_only) { read_only = BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN; -- 2.19.1 Commit e2b8247a322 introduced an error path in qemu_rbd_open() after calling rbd_open(), but neglected to close the image again in this error path. The error path should contain everything that the regular close function qemu_rbd_close() contains. This adds the missing rbd_close() call. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/rbd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/rbd.c b/block/rbd.c index XXXXXXX..XXXXXXX 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -XXX,XX +XXX,XX @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, "automatically marking the image read-only."); r = bdrv_set_read_only(bs, true, &local_err); if (r < 0) { + rbd_close(s->image); error_propagate(errp, local_err); goto failed_open; } -- 2.19.1 Some block drivers have traditionally changed their node to read-only mode without asking the user. This behaviour has been marked deprecated since 2.11, expecting users to provide an explicit read-only=on option. Now that we have auto-read-only=on, enable these drivers to make use of the option. This is the only use of bdrv_set_read_only(), so we can make it a bit more specific and turn it into a bdrv_apply_auto_read_only() that is more convenient for drivers to use. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- include/block/block.h | 3 ++- block.c | 42 +++++++++++++++++++++++++++--------------- block/bochs.c | 17 ++++++----------- block/cloop.c | 16 +++++----------- block/dmg.c | 16 +++++----------- block/rbd.c | 15 ++++----------- block/vvfat.c | 10 ++-------- 7 files changed, 51 insertions(+), 68 deletions(-) diff --git a/include/block/block.h b/include/block/block.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -XXX,XX +XXX,XX @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, bool bdrv_is_read_only(BlockDriverState *bs); int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, bool ignore_allow_rdw, Error **errp); -int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp); +int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg, + Error **errp); bool bdrv_is_writable(BlockDriverState *bs); bool bdrv_is_sg(BlockDriverState *bs); bool bdrv_is_inserted(BlockDriverState *bs); diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, return 0; } -/* TODO Remove (deprecated since 2.11) - * Block drivers are not supposed to automatically change bs->read_only. - * Instead, they should just check whether they can provide what the user - * explicitly requested and error out if read-write is requested, but they can - * only provide read-only access. */ -int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp) +/* + * Called by a driver that can only provide a read-only image. + * + * Returns 0 if the node is already read-only or it could switch the node to + * read-only because BDRV_O_AUTO_RDONLY is set. + * + * Returns -EACCES if the node is read-write and BDRV_O_AUTO_RDONLY is not set + * or bdrv_can_set_read_only() forbids making the node read-only. If @errmsg + * is not NULL, it is used as the error message for the Error object. + */ +int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg, + Error **errp) { int ret = 0; - ret = bdrv_can_set_read_only(bs, read_only, false, errp); - if (ret < 0) { - return ret; + if (!(bs->open_flags & BDRV_O_RDWR)) { + return 0; + } + if (!(bs->open_flags & BDRV_O_AUTO_RDONLY)) { + goto fail; } - bs->read_only = read_only; - - if (read_only) { - bs->open_flags &= ~BDRV_O_RDWR; - } else { - bs->open_flags |= BDRV_O_RDWR; + ret = bdrv_can_set_read_only(bs, true, false, NULL); + if (ret < 0) { + goto fail; } + bs->read_only = true; + bs->open_flags &= ~BDRV_O_RDWR; + return 0; + +fail: + error_setg(errp, "%s", errmsg ?: "Image is read-only"); + return -EACCES; } void bdrv_get_full_backing_filename_from_filename(const char *backed, diff --git a/block/bochs.c b/block/bochs.c index XXXXXXX..XXXXXXX 100644 --- a/block/bochs.c +++ b/block/bochs.c @@ -XXX,XX +XXX,XX @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags, struct bochs_header bochs; int ret; + /* No write support yet */ + ret = bdrv_apply_auto_read_only(bs, NULL, errp); + if (ret < 0) { + return ret; + } + bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false, errp); if (!bs->file) { return -EINVAL; } - if (!bdrv_is_read_only(bs)) { - error_report("Opening bochs images without an explicit read-only=on " - "option is deprecated. Future versions will refuse to " - "open the image instead of automatically marking the " - "image read-only."); - ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */ - if (ret < 0) { - return ret; - } - } - ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs)); if (ret < 0) { return ret; diff --git a/block/cloop.c b/block/cloop.c index XXXXXXX..XXXXXXX 100644 --- a/block/cloop.c +++ b/block/cloop.c @@ -XXX,XX +XXX,XX @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags, uint32_t offsets_size, max_compressed_block_size = 1, i; int ret; + ret = bdrv_apply_auto_read_only(bs, NULL, errp); + if (ret < 0) { + return ret; + } + bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false, errp); if (!bs->file) { return -EINVAL; } - if (!bdrv_is_read_only(bs)) { - error_report("Opening cloop images without an explicit read-only=on " - "option is deprecated. Future versions will refuse to " - "open the image instead of automatically marking the " - "image read-only."); - ret = bdrv_set_read_only(bs, true, errp); - if (ret < 0) { - return ret; - } - } - /* read header */ ret = bdrv_pread(bs->file, 128, &s->block_size, 4); if (ret < 0) { diff --git a/block/dmg.c b/block/dmg.c index XXXXXXX..XXXXXXX 100644 --- a/block/dmg.c +++ b/block/dmg.c @@ -XXX,XX +XXX,XX @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, int64_t offset; int ret; + ret = bdrv_apply_auto_read_only(bs, NULL, errp); + if (ret < 0) { + return ret; + } + bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false, errp); if (!bs->file) { return -EINVAL; } - if (!bdrv_is_read_only(bs)) { - error_report("Opening dmg images without an explicit read-only=on " - "option is deprecated. Future versions will refuse to " - "open the image instead of automatically marking the " - "image read-only."); - ret = bdrv_set_read_only(bs, true, errp); - if (ret < 0) { - return ret; - } - } - block_module_load_one("dmg-bz2"); s->n_chunks = 0; diff --git a/block/rbd.c b/block/rbd.c index XXXXXXX..XXXXXXX 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -XXX,XX +XXX,XX @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, /* If we are using an rbd snapshot, we must be r/o, otherwise * leave as-is */ if (s->snap != NULL) { - if (!bdrv_is_read_only(bs)) { - error_report("Opening rbd snapshots without an explicit " - "read-only=on option is deprecated. Future versions " - "will refuse to open the image instead of " - "automatically marking the image read-only."); - r = bdrv_set_read_only(bs, true, &local_err); - if (r < 0) { - rbd_close(s->image); - error_propagate(errp, local_err); - goto failed_open; - } + r = bdrv_apply_auto_read_only(bs, "rbd snapshots are read-only", errp); + if (r < 0) { + rbd_close(s->image); + goto failed_open; } } diff --git a/block/vvfat.c b/block/vvfat.c index XXXXXXX..XXXXXXX 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -XXX,XX +XXX,XX @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, "Unable to set VVFAT to 'rw' when drive is read-only"); goto fail; } - } else if (!bdrv_is_read_only(bs)) { - error_report("Opening non-rw vvfat images without an explicit " - "read-only=on option is deprecated. Future versions " - "will refuse to open the image instead of " - "automatically marking the image read-only."); - /* read only is the default for safety */ - ret = bdrv_set_read_only(bs, true, &local_err); + } else { + ret = bdrv_apply_auto_read_only(bs, NULL, errp); if (ret < 0) { - error_propagate(errp, local_err); goto fail; } } -- 2.19.1 If read-only=off, but auto-read-only=on is given, open a read-write NBD connection if the server provides a read-write export, but instead of erroring out for read-only exports, just degrade to read-only. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/nbd-client.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/block/nbd-client.c b/block/nbd-client.c index XXXXXXX..XXXXXXX 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -XXX,XX +XXX,XX @@ int nbd_client_init(BlockDriverState *bs, logout("Failed to negotiate with the NBD server\n"); return ret; } - if (client->info.flags & NBD_FLAG_READ_ONLY && - !bdrv_is_read_only(bs)) { - error_setg(errp, - "request for write access conflicts with read-only export"); - return -EACCES; + if (client->info.flags & NBD_FLAG_READ_ONLY) { + ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp); + if (ret < 0) { + return ret; + } } if (client->info.flags & NBD_FLAG_SEND_FUA) { bs->supported_write_flags = BDRV_REQ_FUA; -- 2.19.1 If read-only=off, but auto-read-only=on is given, open the file read-write if we have the permissions, but instead of erroring out for read-only files, just degrade to read-only. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/file-posix.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/block/file-posix.c b/block/file-posix.c index XXXXXXX..XXXXXXX 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options, s->fd = -1; fd = qemu_open(filename, s->open_flags, 0644); - if (fd < 0) { - ret = -errno; - error_setg_errno(errp, errno, "Could not open '%s'", filename); + ret = fd < 0 ? -errno : 0; + + if (ret == -EACCES || ret == -EROFS) { + /* Try to degrade to read-only, but if it doesn't work, still use the + * normal error message. */ + if (bdrv_apply_auto_read_only(bs, NULL, NULL) == 0) { + bdrv_flags &= ~BDRV_O_RDWR; + raw_parse_flags(bdrv_flags, &s->open_flags); + assert(!(s->open_flags & O_CREAT)); + fd = qemu_open(filename, s->open_flags); + ret = fd < 0 ? -errno : 0; + } + } + + if (ret < 0) { + error_setg_errno(errp, -ret, "Could not open '%s'", filename); if (ret == -EROFS) { ret = -EACCES; } -- 2.19.1 If read-only=off, but auto-read-only=on is given, just degrade to read-only. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/curl.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/block/curl.c b/block/curl.c index XXXXXXX..XXXXXXX 100644 --- a/block/curl.c +++ b/block/curl.c @@ -XXX,XX +XXX,XX @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, const char *protocol_delimiter; int ret; - - if (flags & BDRV_O_RDWR) { - error_setg(errp, "curl block device does not support writes"); - return -EROFS; + ret = bdrv_apply_auto_read_only(bs, "curl driver does not support writes", + errp); + if (ret < 0) { + return ret; } if (!libcurl_initialized) { -- 2.19.1 If read-only=off, but auto-read-only=on is given, open the file read-write if we have the permissions, but instead of erroring out for read-only files, just degrade to read-only. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Niels de Vos <ndevos@redhat.com> --- block/gluster.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -XXX,XX +XXX,XX @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, qemu_gluster_parse_flags(bdrv_flags, &open_flags); s->fd = glfs_open(s->glfs, gconf->path, open_flags); - if (!s->fd) { - ret = -errno; + ret = s->fd ? 0 : -errno; + + if (ret == -EACCES || ret == -EROFS) { + /* Try to degrade to read-only, but if it doesn't work, still use the + * normal error message. */ + if (bdrv_apply_auto_read_only(bs, NULL, NULL) == 0) { + open_flags = (open_flags & ~O_RDWR) | O_RDONLY; + s->fd = glfs_open(s->glfs, gconf->path, open_flags); + ret = s->fd ? 0 : -errno; + } } s->supports_seek_data = qemu_gluster_test_seek(s->fd); -- 2.19.1 If read-only=off, but auto-read-only=on is given, open the volume read-write if we have the permissions, but instead of erroring out for read-only volumes, just degrade to read-only. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/iscsi.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index XXXXXXX..XXXXXXX 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -XXX,XX +XXX,XX @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, /* Check the write protect flag of the LUN if we want to write */ if (iscsilun->type == TYPE_DISK && (flags & BDRV_O_RDWR) && iscsilun->write_protected) { - error_setg(errp, "Cannot open a write protected LUN as read-write"); - ret = -EACCES; - goto out; + ret = bdrv_apply_auto_read_only(bs, "LUN is write protected", errp); + if (ret < 0) { + goto out; + } + flags &= ~BDRV_O_RDWR; } iscsi_readcapacity_sync(iscsilun, &local_err); -- 2.19.1 While we want machine interfaces like -blockdev and QMP blockdev-add to add as little auto-detection as possible so that management tools are explicit about their needs, -drive is a convenience option for human users. Enabling auto-read-only=on by default there enables users to use read-only images for read-only guest devices without having to specify read-only=on explicitly. If they try to attach the image to a read-write device, they will still get an error message. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- blockdev.c | 1 + 1 file changed, 1 insertion(+) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_NO_FLUSH, "off"); qdict_set_default_str(bs_opts, BDRV_OPT_READ_ONLY, read_only ? "on" : "off"); + qdict_set_default_str(bs_opts, BDRV_OPT_AUTO_READ_ONLY, "on"); assert((bdrv_flags & BDRV_O_CACHE_MASK) == 0); if (runstate_check(RUN_STATE_INMIGRATE)) { -- 2.19.1 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- tests/qemu-iotests/232 | 147 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/232.out | 59 +++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 207 insertions(+) create mode 100755 tests/qemu-iotests/232 create mode 100644 tests/qemu-iotests/232.out diff --git a/tests/qemu-iotests/232 b/tests/qemu-iotests/232 new file mode 100755 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/232 @@ -XXX,XX +XXX,XX @@ +#!/bin/bash +# +# Test for auto-read-only +# +# Copyright (C) 2018 Red Hat, Inc. +# +# 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/>. +# + +# creator +owner=kwolf@redhat.com + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default! + +_cleanup() +{ + _cleanup_test_img + rm -f $TEST_IMG.snap +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt generic +_supported_proto file +_supported_os Linux + +function do_run_qemu() +{ + echo Testing: "$@" + ( + if ! test -t 0; then + while read cmd; do + echo $cmd + done + fi + echo quit + ) | $QEMU -nographic -monitor stdio -nodefaults "$@" + echo +} + +function run_qemu() +{ + do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_hmp | + _filter_generated_node_ids | _filter_imgfmt +} + +function run_qemu_info_block() +{ + echo "info block -n" | run_qemu "$@" | grep -e "(file" -e "QEMU_PROG" +} + +size=128M + +_make_test_img $size + +echo +echo "=== -drive with read-write image: read-only/auto-read-only combinations ===" +echo + +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on,auto-read-only=off +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on,auto-read-only=on +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on +echo +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off,auto-read-only=off +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off,auto-read-only=on +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off +echo +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,auto-read-only=off +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,auto-read-only=on +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none + +echo +echo "=== -drive with read-only image: read-only/auto-read-only combinations ===" +echo + +chmod a-w $TEST_IMG + +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on,auto-read-only=off +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on,auto-read-only=on +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on +echo +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off,auto-read-only=off +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off,auto-read-only=on +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off +echo +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,auto-read-only=off +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,auto-read-only=on +run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none + +echo +echo "=== -blockdev with read-write image: read-only/auto-read-only combinations ===" +echo + +chmod a+w $TEST_IMG + +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=on,auto-read-only=off +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=on,auto-read-only=on +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=on +echo +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=off,auto-read-only=off +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=off,auto-read-only=on +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=off +echo +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,auto-read-only=off +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,auto-read-only=on +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0 + +echo +echo "=== -blockdev with read-only image: read-only/auto-read-only combinations ===" +echo + +chmod a-w $TEST_IMG + +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=on,auto-read-only=off +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=on,auto-read-only=on +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=on +echo +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=off,auto-read-only=off +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=off,auto-read-only=on +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=off +echo +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,auto-read-only=off +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,auto-read-only=on +run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0 + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/232.out b/tests/qemu-iotests/232.out new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/232.out @@ -XXX,XX +XXX,XX @@ +QA output created by 232 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 + +=== -drive with read-write image: read-only/auto-read-only combinations === + +NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only) +NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only) +NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only) + +NODE_NAME: TEST_DIR/t.IMGFMT (file) +NODE_NAME: TEST_DIR/t.IMGFMT (file) +NODE_NAME: TEST_DIR/t.IMGFMT (file) + +NODE_NAME: TEST_DIR/t.IMGFMT (file) +NODE_NAME: TEST_DIR/t.IMGFMT (file) +NODE_NAME: TEST_DIR/t.IMGFMT (file) + +=== -drive with read-only image: read-only/auto-read-only combinations === + +NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only) +NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only) +NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only) + +QEMU_PROG: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none,read-only=off,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied +NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only) +NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only) + +QEMU_PROG: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied +NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only) +NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only) + +=== -blockdev with read-write image: read-only/auto-read-only combinations === + +node0: TEST_DIR/t.IMGFMT (file, read-only) +node0: TEST_DIR/t.IMGFMT (file, read-only) +node0: TEST_DIR/t.IMGFMT (file, read-only) + +node0: TEST_DIR/t.IMGFMT (file) +node0: TEST_DIR/t.IMGFMT (file) +node0: TEST_DIR/t.IMGFMT (file) + +node0: TEST_DIR/t.IMGFMT (file) +node0: TEST_DIR/t.IMGFMT (file) +node0: TEST_DIR/t.IMGFMT (file) + +=== -blockdev with read-only image: read-only/auto-read-only combinations === + +node0: TEST_DIR/t.IMGFMT (file, read-only) +node0: TEST_DIR/t.IMGFMT (file, read-only) +node0: TEST_DIR/t.IMGFMT (file, read-only) + +QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,read-only=off,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied +node0: TEST_DIR/t.IMGFMT (file, read-only) +QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied + +QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied +node0: TEST_DIR/t.IMGFMT (file, read-only) +QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0: Could not open 'TEST_DIR/t.IMGFMT': Permission denied +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -XXX,XX +XXX,XX @@ 227 auto quick 229 auto quick 231 auto quick +232 auto quick -- 2.19.1 From: Max Reitz <mreitz@redhat.com> This adds some whitespace into the option help (including indentation) and puts angle brackets around the type names. Furthermore, the list name is no longer printed as part of every line, but only once in advance, and only if the caller did not print a caption already. This patch also restores the description alignment we had before commit 9cbef9d68ee1d8d0, just at 24 instead of 16 characters like we used to. This increase is because now we have the type and two spaces of indentation before the description, and with a usual type name length of three chracters, this sums up to eight additional characters -- which means that we now need 24 characters to get the same amount of padding for most options. Also, 24 is a third of 80, which makes it kind of a round number in terminal terms. Finally, this patch amends the reference output of iotest 082 to match the changes (and thus makes it pass again). Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/option.h | 2 +- qemu-img.c | 4 +- util/qemu-option.c | 32 +- tests/qemu-iotests/082.out | 956 ++++++++++++++++++------------------- 4 files changed, 507 insertions(+), 487 deletions(-) diff --git a/include/qemu/option.h b/include/qemu/option.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -XXX,XX +XXX,XX @@ typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Error **errp); int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque, Error **errp); void qemu_opts_print(QemuOpts *opts, const char *sep); -void qemu_opts_print_help(QemuOptsList *list); +void qemu_opts_print_help(QemuOptsList *list, bool print_caption); void qemu_opts_free(QemuOptsList *list); QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list); diff --git a/qemu-img.c b/qemu-img.c index XXXXXXX..XXXXXXX 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -XXX,XX +XXX,XX @@ static int print_block_option_help(const char *filename, const char *fmt) } printf("Supported options:\n"); - qemu_opts_print_help(create_opts); + qemu_opts_print_help(create_opts, false); qemu_opts_free(create_opts); return 0; } @@ -XXX,XX +XXX,XX @@ static int print_amend_option_help(const char *format) assert(drv->create_opts); printf("Creation options for '%s':\n", format); - qemu_opts_print_help(drv->create_opts); + qemu_opts_print_help(drv->create_opts, false); printf("\nNote that not all of these options may be amendable.\n"); return 0; } diff --git a/util/qemu-option.c b/util/qemu-option.c index XXXXXXX..XXXXXXX 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -XXX,XX +XXX,XX @@ static const char *opt_type_to_string(enum QemuOptType type) g_assert_not_reached(); } -void qemu_opts_print_help(QemuOptsList *list) +/** + * Print the list of options available in the given list. If + * @print_caption is true, a caption (including the list name, if it + * exists) is printed. The options itself will be indented, so + * @print_caption should only be set to false if the caller prints its + * own custom caption (so that the indentation makes sense). + */ +void qemu_opts_print_help(QemuOptsList *list, bool print_caption) { QemuOptDesc *desc; int i; @@ -XXX,XX +XXX,XX @@ void qemu_opts_print_help(QemuOptsList *list) desc = list->desc; while (desc && desc->name) { GString *str = g_string_new(NULL); - if (list->name) { - g_string_append_printf(str, "%s.", list->name); - } - g_string_append_printf(str, "%s=%s", desc->name, + g_string_append_printf(str, " %s=<%s>", desc->name, opt_type_to_string(desc->type)); if (desc->help) { + if (str->len < 24) { + g_string_append_printf(str, "%*s", 24 - (int)str->len, ""); + } g_string_append_printf(str, " - %s", desc->help); } g_ptr_array_add(array, g_string_free(str, false)); @@ -XXX,XX +XXX,XX @@ void qemu_opts_print_help(QemuOptsList *list) } g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0); + if (print_caption && array->len > 0) { + if (list->name) { + printf("%s options:\n", list->name); + } else { + printf("Options:\n"); + } + } else if (array->len == 0) { + if (list->name) { + printf("There are no options for %s.\n", list->name); + } else { + printf("No options available.\n"); + } + } for (i = 0; i < array->len; i++) { printf("%s\n", (char *)array->pdata[i]); } @@ -XXX,XX +XXX,XX @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params, opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err); if (err) { if (invalidp && has_help_option(params)) { - qemu_opts_print_help(list); + qemu_opts_print_help(list, true); error_free(err); } else { error_report_err(err); diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/082.out +++ b/tests/qemu-iotests/082.out @@ -XXX,XX +XXX,XX @@ cluster_size: 8192 Testing: create -f qcow2 -o help TEST_DIR/t.qcow2 128M Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: create -f qcow2 -o ? TEST_DIR/t.qcow2 128M Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: create -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 128M Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: create -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 128M Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: create -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 128M Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: create -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 128M Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: create -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 128M Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: create -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 128M Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: create -f qcow2 -u -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 128M Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=134217728 backing_file=TEST_DIR/t.qcow2,,help cluster_size=65536 lazy_refcounts=off refcount_bits=16 @@ -XXX,XX +XXX,XX @@ qemu-img: Invalid option list: ,, Testing: create -f qcow2 -o help Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: create -o help Supported options: -size Virtual disk size + size=<size> - Virtual disk size Testing: create -f bochs -o help qemu-img: Format driver 'bochs' does not support image creation @@ -XXX,XX +XXX,XX @@ cluster_size: 8192 Testing: convert -O qcow2 -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: convert -O qcow2 -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: convert -O qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: convert -O qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: convert -O qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: convert -O qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: convert -O qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: convert -O qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits -nocow Turn off copy-on-write (valid only on btrfs) + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + nocow=<bool (on/off)> - Turn off copy-on-write (valid only on btrfs) + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base qemu-img: Could not open 'TEST_DIR/t.qcow2.base': Could not open backing file: Could not open 'TEST_DIR/t.qcow2,help': No such file or directory @@ -XXX,XX +XXX,XX @@ qemu-img: Invalid option list: ,, Testing: convert -O qcow2 -o help Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Testing: convert -o help Supported options: -size Virtual disk size + size=<size> - Virtual disk size Testing: convert -O bochs -o help qemu-img: Format driver 'bochs' does not support image creation @@ -XXX,XX +XXX,XX @@ cluster_size: 65536 Testing: amend -f qcow2 -o help TEST_DIR/t.qcow2 Creation options for 'qcow2': -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Note that not all of these options may be amendable. Testing: amend -f qcow2 -o ? TEST_DIR/t.qcow2 Creation options for 'qcow2': -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Note that not all of these options may be amendable. Testing: amend -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 Creation options for 'qcow2': -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Note that not all of these options may be amendable. Testing: amend -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 Creation options for 'qcow2': -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Note that not all of these options may be amendable. Testing: amend -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 Creation options for 'qcow2': -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Note that not all of these options may be amendable. Testing: amend -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 Creation options for 'qcow2': -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Note that not all of these options may be amendable. Testing: amend -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 Creation options for 'qcow2': -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Note that not all of these options may be amendable. Testing: amend -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 Creation options for 'qcow2': -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Note that not all of these options may be amendable. @@ -XXX,XX +XXX,XX @@ qemu-img: Invalid option list: ,, Testing: amend -f qcow2 -o help Creation options for 'qcow2': -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocation Preallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bits Width of a reference count entry in bits + backing_file=<str> - File name of a base image + backing_fmt=<str> - Image format of the base image + cluster_size=<size> - qcow2 cluster size + compat=<str> - Compatibility level (0.10 or 1.1) + encrypt.cipher-alg=<str> - Name of encryption cipher algorithm + encrypt.cipher-mode=<str> - Name of encryption cipher mode + encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks' + encrypt.hash-alg=<str> - Name of encryption hash algorithm + encrypt.iter-time=<num> - Time to spend in PBKDF in milliseconds + encrypt.ivgen-alg=<str> - Name of IV generator algorithm + encrypt.ivgen-hash-alg=<str> - Name of IV generator hash algorithm + encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase + encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) + lazy_refcounts=<bool (on/off)> - Postpone refcount updates + preallocation=<str> - Preallocation mode (allowed values: off, metadata, falloc, full) + refcount_bits=<num> - Width of a reference count entry in bits + size=<size> - Virtual disk size Note that not all of these options may be amendable. Testing: convert -o help Supported options: -size Virtual disk size + size=<size> - Virtual disk size Testing: amend -f bochs -o help qemu-img: Format driver 'bochs' does not support option amendment -- 2.19.1 From: Max Reitz <mreitz@redhat.com> Following the example of qemu_opts_print_help(), indent all entries in the list of character devices. Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- chardev/char.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chardev/char.c b/chardev/char.c index XXXXXXX..XXXXXXX 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -XXX,XX +XXX,XX @@ help_string_append(const char *name, void *opaque) { GString *str = opaque; - g_string_append_printf(str, "\n%s", name); + g_string_append_printf(str, "\n %s", name); } static const char *chardev_alias_translate(const char *name) -- 2.19.1 From: Max Reitz <mreitz@redhat.com> Just like in qemu_opts_print_help(), print the device name as a caption instead of on every single line, indent all options, add angle brackets around types, and align the descriptions after 24 characters. Also, separate the descriptions with " - " instead of putting them in parentheses, because that is what we do everywhere else. This does look a bit funny here because basically all bits have the description "on/off", but funny does not mean it is less readable. Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qdev-monitor.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/qdev-monitor.c b/qdev-monitor.c index XXXXXXX..XXXXXXX 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -XXX,XX +XXX,XX @@ int qdev_device_help(QemuOpts *opts) goto error; } + if (prop_list) { + out_printf("%s options:\n", driver); + } else { + out_printf("There are no options for %s.\n", driver); + } for (prop = prop_list; prop; prop = prop->next) { - out_printf("%s.%s=%s", driver, prop->value->name, prop->value->type); + int len; + out_printf(" %s=<%s>%n", prop->value->name, prop->value->type, &len); if (prop->value->has_description) { - out_printf(" (%s)\n", prop->value->description); + if (len < 24) { + out_printf("%*s", 24 - len, ""); + } + out_printf(" - %s\n", prop->value->description); } else { out_printf("\n"); } -- 2.19.1 From: Max Reitz <mreitz@redhat.com> Just like in qemu_opts_print_help(), print the object name as a caption instead of on every single line, indent all options, add angle brackets around types, and align the descriptions after 24 characters. Also, indent every object name in the list of available objects. Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- vl.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/vl.c b/vl.c index XXXXXXX..XXXXXXX 100644 --- a/vl.c +++ b/vl.c @@ -XXX,XX +XXX,XX @@ static bool object_create_initial(const char *type, QemuOpts *opts) list = object_class_get_list_sorted(TYPE_USER_CREATABLE, false); for (l = list; l != NULL; l = l->next) { ObjectClass *oc = OBJECT_CLASS(l->data); - printf("%s\n", object_class_get_name(oc)); + printf(" %s\n", object_class_get_name(oc)); } g_slist_free(list); exit(0); @@ -XXX,XX +XXX,XX @@ static bool object_create_initial(const char *type, QemuOpts *opts) } str = g_string_new(NULL); - g_string_append_printf(str, "%s.%s=%s", type, - prop->name, prop->type); + g_string_append_printf(str, " %s=<%s>", prop->name, prop->type); if (prop->description) { + if (str->len < 24) { + g_string_append_printf(str, "%*s", 24 - (int)str->len, ""); + } g_string_append_printf(str, " - %s", prop->description); } g_ptr_array_add(array, g_string_free(str, false)); } g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0); + if (array->len > 0) { + printf("%s options:\n", type); + } else { + printf("There are no options for %s.\n", type); + } for (i = 0; i < array->len; i++) { printf("%s\n", (char *)array->pdata[i]); } -- 2.19.1 From: Max Reitz <mreitz@redhat.com> There is no good reason why there should be a newline in this description, so remove it. Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- vl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vl.c b/vl.c index XXXXXXX..XXXXXXX 100644 --- a/vl.c +++ b/vl.c @@ -XXX,XX +XXX,XX @@ static QemuOptsList qemu_fw_cfg_opts = { }, { .name = "file", .type = QEMU_OPT_STRING, - .help = "Sets the name of the file from which\n" + .help = "Sets the name of the file from which " "the fw_cfg blob will be loaded", }, { .name = "string", -- 2.19.1 From: Leonid Bloch <lbloch@janustech.com> If an expression is used to define DEFAULT_CLUSTER_SIZE, when compiled, it will be embedded as a literal expression in the binary (as the default value) because it is stringified to mark the size of the default value. Now this is fixed by using a defined number to define this value. Signed-off-by: Leonid Bloch <lbloch@janustech.com> Reviewed-by: Stefan Weil <sw@weilnetz.de> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vdi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block/vdi.c b/block/vdi.c index XXXXXXX..XXXXXXX 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -XXX,XX +XXX,XX @@ #define BLOCK_OPT_STATIC "static" #define SECTOR_SIZE 512 -#define DEFAULT_CLUSTER_SIZE (1 * MiB) +#define DEFAULT_CLUSTER_SIZE S_1MiB #if defined(CONFIG_VDI_DEBUG) #define VDI_DEBUG 1 @@ -XXX,XX +XXX,XX @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } else if (header.block_size != DEFAULT_CLUSTER_SIZE) { error_setg(errp, "unsupported VDI image (block size %" PRIu32 - " is not %" PRIu64 ")", + " is not %" PRIu32 ")", header.block_size, DEFAULT_CLUSTER_SIZE); ret = -ENOTSUP; goto fail; -- 2.19.1 From: Leonid Bloch <lbloch@janustech.com> The lookup table for power-of-two sizes was added in commit 540b8492618eb for the purpose of having convenient shortcuts for these sizes in cases when the literal number has to be present at compile time, and expressions as '(1 * KiB)' can not be used. One such case is the stringification of sizes. Beyond that, it is convenient to use these shortcuts for all power-of-two sizes, even if they don't have to be literal numbers. Despite its convenience, this table introduced 55 lines of "dumb" code, the purpose and origin of which are obscure without reading the message of the commit which introduced it. This patch fixes that by adding a comment to the code itself with a brief explanation for the reasoning behind this table. This comment includes the short AWK script that generated the table, so that anyone who's interested could make sure that the values in it are correct (otherwise these values look as if they were typed manually). Signed-off-by: Leonid Bloch <lbloch@janustech.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/units.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/include/qemu/units.h b/include/qemu/units.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/units.h +++ b/include/qemu/units.h @@ -XXX,XX +XXX,XX @@ #define PiB (INT64_C(1) << 50) #define EiB (INT64_C(1) << 60) +/* + * The following lookup table is intended to be used when a literal string of + * the number of bytes is required (for example if it needs to be stringified). + * It can also be used for generic shortcuts of power-of-two sizes. + * This table is generated using the AWK script below: + * + * BEGIN { + * suffix="KMGTPE"; + * for(i=10; i<64; i++) { + * val=2**i; + * s=substr(suffix, int(i/10), 1); + * n=2**(i%10); + * pad=21-int(log(n)/log(10)); + * printf("#define S_%d%siB %*d\n", n, s, pad, val); + * } + * } + */ + #define S_1KiB 1024 #define S_2KiB 2048 #define S_4KiB 4096 -- 2.19.1 The following changes since commit 6972ef1440a9d685482d78672620a7482f2bd09a: Merge tag 'pull-tcg-20230516-3' of https://gitlab.com/rth7680/qemu into staging (2023-05-16 21:30:27 -0700) are available in the Git repository at: https://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to 75b2591bbce5dc9f3da89f140b7bdc00e92fa8ec: tested: add test for nested aio_poll() in poll handlers (2023-05-17 18:01:22 +0200) ---------------------------------------------------------------- Block layer patches - qcow2 spec: Rename "zlib" compression to "deflate" - Honour graph read lock even in the main thread + prerequisite fixes - aio-posix: do not nest poll handlers (fixes infinite recursion) - Refactor QMP blockdev transactions - iotests/245: Check if 'compress' driver is available ---------------------------------------------------------------- Akihiro Suda (1): docs/interop/qcow2.txt: fix description about "zlib" clusters Kevin Wolf (9): block: Call .bdrv_co_create(_opts) unlocked block/export: Fix null pointer dereference in error path qcow2: Unlock the graph in qcow2_do_open() where necessary qemu-img: Take graph lock more selectively test-bdrv-drain: Take graph lock more selectively test-bdrv-drain: Call bdrv_co_unref() in coroutine context blockjob: Adhere to rate limit even when reentered early graph-lock: Honour read locks even in the main thread iotests/245: Check if 'compress' driver is available Stefan Hajnoczi (2): aio-posix: do not nest poll handlers tested: add test for nested aio_poll() in poll handlers Vladimir Sementsov-Ogievskiy (6): blockdev: refactor transaction to use Transaction API blockdev: transactions: rename some things blockdev: qmp_transaction: refactor loop to classic for blockdev: transaction: refactor handling transaction properties blockdev: use state.bitmap in block-dirty-bitmap-add action blockdev: qmp_transaction: drop extra generic layer docs/interop/qcow2.txt | 10 +- include/block/block-global-state.h | 8 +- include/block/block_int-common.h | 4 +- include/block/blockjob_int.h | 14 +- block.c | 1 - block/commit.c | 7 +- block/create.c | 1 - block/crypto.c | 25 +- block/export/export.c | 6 +- block/graph-lock.c | 10 - block/mirror.c | 23 +- block/parallels.c | 6 +- block/qcow.c | 6 +- block/qcow2.c | 43 ++- block/qed.c | 6 +- block/raw-format.c | 2 +- block/stream.c | 7 +- block/vdi.c | 11 +- block/vhdx.c | 8 +- block/vmdk.c | 27 +- block/vpc.c | 6 +- blockdev.c | 606 +++++++++++++++---------------------- blockjob.c | 22 +- qemu-img.c | 5 +- tests/unit/test-bdrv-drain.c | 6 +- tests/unit/test-nested-aio-poll.c | 130 ++++++++ util/aio-posix.c | 11 + tests/qemu-iotests/245 | 7 +- tests/qemu-iotests/245.out | 9 +- tests/unit/meson.build | 1 + 30 files changed, 541 insertions(+), 487 deletions(-) create mode 100644 tests/unit/test-nested-aio-poll.c From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> We are going to add more block-graph modifying transaction actions, and block-graph modifying functions are already based on Transaction API. Next, we'll need to separately update permissions after several graph-modifying actions, and this would be simple with help of Transaction API. So, now let's just transform what we have into new-style transaction actions. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20230510150624.310640-2-vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 309 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 178 insertions(+), 131 deletions(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ typedef struct BlkActionState BlkActionState; */ typedef struct BlkActionOps { size_t instance_size; - void (*prepare)(BlkActionState *common, Error **errp); - void (*commit)(BlkActionState *common); - void (*abort)(BlkActionState *common); - void (*clean)(BlkActionState *common); + void (*action)(BlkActionState *common, Transaction *tran, Error **errp); } BlkActionOps; /** @@ -XXX,XX +XXX,XX @@ typedef struct InternalSnapshotState { bool created; } InternalSnapshotState; +static void internal_snapshot_abort(void *opaque); +static void internal_snapshot_clean(void *opaque); +TransactionActionDrv internal_snapshot_drv = { + .abort = internal_snapshot_abort, + .clean = internal_snapshot_clean, +}; static int action_check_completion_mode(BlkActionState *s, Error **errp) { @@ -XXX,XX +XXX,XX @@ static int action_check_completion_mode(BlkActionState *s, Error **errp) return 0; } -static void internal_snapshot_prepare(BlkActionState *common, - Error **errp) +static void internal_snapshot_action(BlkActionState *common, + Transaction *tran, Error **errp) { Error *local_err = NULL; const char *device; @@ -XXX,XX +XXX,XX @@ static void internal_snapshot_prepare(BlkActionState *common, internal = common->action->u.blockdev_snapshot_internal_sync.data; state = DO_UPCAST(InternalSnapshotState, common, common); + tran_add(tran, &internal_snapshot_drv, state); + /* 1. parse input */ device = internal->device; name = internal->name; @@ -XXX,XX +XXX,XX @@ out: aio_context_release(aio_context); } -static void internal_snapshot_abort(BlkActionState *common) +static void internal_snapshot_abort(void *opaque) { - InternalSnapshotState *state = - DO_UPCAST(InternalSnapshotState, common, common); + InternalSnapshotState *state = opaque; BlockDriverState *bs = state->bs; QEMUSnapshotInfo *sn = &state->sn; AioContext *aio_context; @@ -XXX,XX +XXX,XX @@ static void internal_snapshot_abort(BlkActionState *common) aio_context_release(aio_context); } -static void internal_snapshot_clean(BlkActionState *common) +static void internal_snapshot_clean(void *opaque) { - InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState, - common, common); + g_autofree InternalSnapshotState *state = opaque; AioContext *aio_context; if (!state->bs) { @@ -XXX,XX +XXX,XX @@ typedef struct ExternalSnapshotState { bool overlay_appended; } ExternalSnapshotState; -static void external_snapshot_prepare(BlkActionState *common, - Error **errp) +static void external_snapshot_commit(void *opaque); +static void external_snapshot_abort(void *opaque); +static void external_snapshot_clean(void *opaque); +TransactionActionDrv external_snapshot_drv = { + .commit = external_snapshot_commit, + .abort = external_snapshot_abort, + .clean = external_snapshot_clean, +}; + +static void external_snapshot_action(BlkActionState *common, Transaction *tran, + Error **errp) { int ret; int flags = 0; @@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common, AioContext *aio_context; uint64_t perm, shared; + tran_add(tran, &external_snapshot_drv, state); + /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar * purpose but a different set of parameters */ switch (action->type) { @@ -XXX,XX +XXX,XX @@ out: aio_context_release(aio_context); } -static void external_snapshot_commit(BlkActionState *common) +static void external_snapshot_commit(void *opaque) { - ExternalSnapshotState *state = - DO_UPCAST(ExternalSnapshotState, common, common); + ExternalSnapshotState *state = opaque; AioContext *aio_context; aio_context = bdrv_get_aio_context(state->old_bs); @@ -XXX,XX +XXX,XX @@ static void external_snapshot_commit(BlkActionState *common) aio_context_release(aio_context); } -static void external_snapshot_abort(BlkActionState *common) +static void external_snapshot_abort(void *opaque) { - ExternalSnapshotState *state = - DO_UPCAST(ExternalSnapshotState, common, common); + ExternalSnapshotState *state = opaque; if (state->new_bs) { if (state->overlay_appended) { AioContext *aio_context; @@ -XXX,XX +XXX,XX @@ static void external_snapshot_abort(BlkActionState *common) } } -static void external_snapshot_clean(BlkActionState *common) +static void external_snapshot_clean(void *opaque) { - ExternalSnapshotState *state = - DO_UPCAST(ExternalSnapshotState, common, common); + g_autofree ExternalSnapshotState *state = opaque; AioContext *aio_context; if (!state->old_bs) { @@ -XXX,XX +XXX,XX @@ static BlockJob *do_backup_common(BackupCommon *backup, AioContext *aio_context, JobTxn *txn, Error **errp); -static void drive_backup_prepare(BlkActionState *common, Error **errp) +static void drive_backup_commit(void *opaque); +static void drive_backup_abort(void *opaque); +static void drive_backup_clean(void *opaque); +TransactionActionDrv drive_backup_drv = { + .commit = drive_backup_commit, + .abort = drive_backup_abort, + .clean = drive_backup_clean, +}; + +static void drive_backup_action(BlkActionState *common, Transaction *tran, + Error **errp) { DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); DriveBackup *backup; @@ -XXX,XX +XXX,XX @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) bool set_backing_hd = false; int ret; + tran_add(tran, &drive_backup_drv, state); + assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP); backup = common->action->u.drive_backup.data; @@ -XXX,XX +XXX,XX @@ out: aio_context_release(aio_context); } -static void drive_backup_commit(BlkActionState *common) +static void drive_backup_commit(void *opaque) { - DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); + DriveBackupState *state = opaque; AioContext *aio_context; aio_context = bdrv_get_aio_context(state->bs); @@ -XXX,XX +XXX,XX @@ static void drive_backup_commit(BlkActionState *common) aio_context_release(aio_context); } -static void drive_backup_abort(BlkActionState *common) +static void drive_backup_abort(void *opaque) { - DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); + DriveBackupState *state = opaque; if (state->job) { job_cancel_sync(&state->job->job, true); } } -static void drive_backup_clean(BlkActionState *common) +static void drive_backup_clean(void *opaque) { - DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); + g_autofree DriveBackupState *state = opaque; AioContext *aio_context; if (!state->bs) { @@ -XXX,XX +XXX,XX @@ typedef struct BlockdevBackupState { BlockJob *job; } BlockdevBackupState; -static void blockdev_backup_prepare(BlkActionState *common, Error **errp) +static void blockdev_backup_commit(void *opaque); +static void blockdev_backup_abort(void *opaque); +static void blockdev_backup_clean(void *opaque); +TransactionActionDrv blockdev_backup_drv = { + .commit = blockdev_backup_commit, + .abort = blockdev_backup_abort, + .clean = blockdev_backup_clean, +}; + +static void blockdev_backup_action(BlkActionState *common, Transaction *tran, + Error **errp) { BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); BlockdevBackup *backup; @@ -XXX,XX +XXX,XX @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) AioContext *old_context; int ret; + tran_add(tran, &blockdev_backup_drv, state); + assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP); backup = common->action->u.blockdev_backup.data; @@ -XXX,XX +XXX,XX @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) aio_context_release(aio_context); } -static void blockdev_backup_commit(BlkActionState *common) +static void blockdev_backup_commit(void *opaque) { - BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); + BlockdevBackupState *state = opaque; AioContext *aio_context; aio_context = bdrv_get_aio_context(state->bs); @@ -XXX,XX +XXX,XX @@ static void blockdev_backup_commit(BlkActionState *common) aio_context_release(aio_context); } -static void blockdev_backup_abort(BlkActionState *common) +static void blockdev_backup_abort(void *opaque) { - BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); + BlockdevBackupState *state = opaque; if (state->job) { job_cancel_sync(&state->job->job, true); } } -static void blockdev_backup_clean(BlkActionState *common) +static void blockdev_backup_clean(void *opaque) { - BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); + g_autofree BlockdevBackupState *state = opaque; AioContext *aio_context; if (!state->bs) { @@ -XXX,XX +XXX,XX @@ typedef struct BlockDirtyBitmapState { bool was_enabled; } BlockDirtyBitmapState; -static void block_dirty_bitmap_add_prepare(BlkActionState *common, - Error **errp) +static void block_dirty_bitmap_add_abort(void *opaque); +TransactionActionDrv block_dirty_bitmap_add_drv = { + .abort = block_dirty_bitmap_add_abort, + .clean = g_free, +}; + +static void block_dirty_bitmap_add_action(BlkActionState *common, + Transaction *tran, Error **errp) { Error *local_err = NULL; BlockDirtyBitmapAdd *action; BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, common, common); + tran_add(tran, &block_dirty_bitmap_add_drv, state); + if (action_check_completion_mode(common, errp) < 0) { return; } @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common, } } -static void block_dirty_bitmap_add_abort(BlkActionState *common) +static void block_dirty_bitmap_add_abort(void *opaque) { BlockDirtyBitmapAdd *action; - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); + BlockDirtyBitmapState *state = opaque; - action = common->action->u.block_dirty_bitmap_add.data; + action = state->common.action->u.block_dirty_bitmap_add.data; /* Should not be able to fail: IF the bitmap was added via .prepare(), * then the node reference and bitmap name must have been valid. */ @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_add_abort(BlkActionState *common) } } -static void block_dirty_bitmap_clear_prepare(BlkActionState *common, - Error **errp) +static void block_dirty_bitmap_restore(void *opaque); +static void block_dirty_bitmap_free_backup(void *opaque); +TransactionActionDrv block_dirty_bitmap_clear_drv = { + .abort = block_dirty_bitmap_restore, + .commit = block_dirty_bitmap_free_backup, + .clean = g_free, +}; + +static void block_dirty_bitmap_clear_action(BlkActionState *common, + Transaction *tran, Error **errp) { BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, common, common); BlockDirtyBitmap *action; + tran_add(tran, &block_dirty_bitmap_clear_drv, state); + if (action_check_completion_mode(common, errp) < 0) { return; } @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); } -static void block_dirty_bitmap_restore(BlkActionState *common) +static void block_dirty_bitmap_restore(void *opaque) { - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); + BlockDirtyBitmapState *state = opaque; if (state->backup) { bdrv_restore_dirty_bitmap(state->bitmap, state->backup); } } -static void block_dirty_bitmap_free_backup(BlkActionState *common) +static void block_dirty_bitmap_free_backup(void *opaque) { - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); + BlockDirtyBitmapState *state = opaque; hbitmap_free(state->backup); } -static void block_dirty_bitmap_enable_prepare(BlkActionState *common, - Error **errp) +static void block_dirty_bitmap_enable_abort(void *opaque); +TransactionActionDrv block_dirty_bitmap_enable_drv = { + .abort = block_dirty_bitmap_enable_abort, + .clean = g_free, +}; + +static void block_dirty_bitmap_enable_action(BlkActionState *common, + Transaction *tran, Error **errp) { BlockDirtyBitmap *action; BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, common, common); + tran_add(tran, &block_dirty_bitmap_enable_drv, state); + if (action_check_completion_mode(common, errp) < 0) { return; } @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common, bdrv_enable_dirty_bitmap(state->bitmap); } -static void block_dirty_bitmap_enable_abort(BlkActionState *common) +static void block_dirty_bitmap_enable_abort(void *opaque) { - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); + BlockDirtyBitmapState *state = opaque; if (!state->was_enabled) { bdrv_disable_dirty_bitmap(state->bitmap); } } -static void block_dirty_bitmap_disable_prepare(BlkActionState *common, - Error **errp) +static void block_dirty_bitmap_disable_abort(void *opaque); +TransactionActionDrv block_dirty_bitmap_disable_drv = { + .abort = block_dirty_bitmap_disable_abort, + .clean = g_free, +}; + +static void block_dirty_bitmap_disable_action(BlkActionState *common, + Transaction *tran, Error **errp) { BlockDirtyBitmap *action; BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, common, common); + tran_add(tran, &block_dirty_bitmap_disable_drv, state); + if (action_check_completion_mode(common, errp) < 0) { return; } @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common, bdrv_disable_dirty_bitmap(state->bitmap); } -static void block_dirty_bitmap_disable_abort(BlkActionState *common) +static void block_dirty_bitmap_disable_abort(void *opaque) { - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); + BlockDirtyBitmapState *state = opaque; if (state->was_enabled) { bdrv_enable_dirty_bitmap(state->bitmap); } } -static void block_dirty_bitmap_merge_prepare(BlkActionState *common, - Error **errp) +TransactionActionDrv block_dirty_bitmap_merge_drv = { + .commit = block_dirty_bitmap_free_backup, + .abort = block_dirty_bitmap_restore, + .clean = g_free, +}; + +static void block_dirty_bitmap_merge_action(BlkActionState *common, + Transaction *tran, Error **errp) { BlockDirtyBitmapMerge *action; BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, common, common); + tran_add(tran, &block_dirty_bitmap_merge_drv, state); + if (action_check_completion_mode(common, errp) < 0) { return; } @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common, errp); } -static void block_dirty_bitmap_remove_prepare(BlkActionState *common, - Error **errp) +static void block_dirty_bitmap_remove_commit(void *opaque); +static void block_dirty_bitmap_remove_abort(void *opaque); +TransactionActionDrv block_dirty_bitmap_remove_drv = { + .commit = block_dirty_bitmap_remove_commit, + .abort = block_dirty_bitmap_remove_abort, + .clean = g_free, +}; + +static void block_dirty_bitmap_remove_action(BlkActionState *common, + Transaction *tran, Error **errp) { BlockDirtyBitmap *action; BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, common, common); + tran_add(tran, &block_dirty_bitmap_remove_drv, state); + if (action_check_completion_mode(common, errp) < 0) { return; } @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_remove_prepare(BlkActionState *common, } } -static void block_dirty_bitmap_remove_abort(BlkActionState *common) +static void block_dirty_bitmap_remove_abort(void *opaque) { - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); + BlockDirtyBitmapState *state = opaque; if (state->bitmap) { bdrv_dirty_bitmap_skip_store(state->bitmap, false); @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_remove_abort(BlkActionState *common) } } -static void block_dirty_bitmap_remove_commit(BlkActionState *common) +static void block_dirty_bitmap_remove_commit(void *opaque) { - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); + BlockDirtyBitmapState *state = opaque; bdrv_dirty_bitmap_set_busy(state->bitmap, false); bdrv_release_dirty_bitmap(state->bitmap); } -static void abort_prepare(BlkActionState *common, Error **errp) +static void abort_commit(void *opaque); +TransactionActionDrv abort_drv = { + .commit = abort_commit, + .clean = g_free, +}; + +static void abort_action(BlkActionState *common, Transaction *tran, + Error **errp) { + tran_add(tran, &abort_drv, common); error_setg(errp, "Transaction aborted using Abort action"); } -static void abort_commit(BlkActionState *common) +static void abort_commit(void *opaque) { g_assert_not_reached(); /* this action never succeeds */ } @@ -XXX,XX +XXX,XX @@ static void abort_commit(BlkActionState *common) static const BlkActionOps actions[] = { [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT] = { .instance_size = sizeof(ExternalSnapshotState), - .prepare = external_snapshot_prepare, - .commit = external_snapshot_commit, - .abort = external_snapshot_abort, - .clean = external_snapshot_clean, + .action = external_snapshot_action, }, [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = { .instance_size = sizeof(ExternalSnapshotState), - .prepare = external_snapshot_prepare, - .commit = external_snapshot_commit, - .abort = external_snapshot_abort, - .clean = external_snapshot_clean, + .action = external_snapshot_action, }, [TRANSACTION_ACTION_KIND_DRIVE_BACKUP] = { .instance_size = sizeof(DriveBackupState), - .prepare = drive_backup_prepare, - .commit = drive_backup_commit, - .abort = drive_backup_abort, - .clean = drive_backup_clean, + .action = drive_backup_action, }, [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = { .instance_size = sizeof(BlockdevBackupState), - .prepare = blockdev_backup_prepare, - .commit = blockdev_backup_commit, - .abort = blockdev_backup_abort, - .clean = blockdev_backup_clean, + .action = blockdev_backup_action, }, [TRANSACTION_ACTION_KIND_ABORT] = { .instance_size = sizeof(BlkActionState), - .prepare = abort_prepare, - .commit = abort_commit, + .action = abort_action, }, [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC] = { .instance_size = sizeof(InternalSnapshotState), - .prepare = internal_snapshot_prepare, - .abort = internal_snapshot_abort, - .clean = internal_snapshot_clean, + .action = internal_snapshot_action, }, [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = { .instance_size = sizeof(BlockDirtyBitmapState), - .prepare = block_dirty_bitmap_add_prepare, - .abort = block_dirty_bitmap_add_abort, + .action = block_dirty_bitmap_add_action, }, [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = { .instance_size = sizeof(BlockDirtyBitmapState), - .prepare = block_dirty_bitmap_clear_prepare, - .commit = block_dirty_bitmap_free_backup, - .abort = block_dirty_bitmap_restore, + .action = block_dirty_bitmap_clear_action, }, [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = { .instance_size = sizeof(BlockDirtyBitmapState), - .prepare = block_dirty_bitmap_enable_prepare, - .abort = block_dirty_bitmap_enable_abort, + .action = block_dirty_bitmap_enable_action, }, [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = { .instance_size = sizeof(BlockDirtyBitmapState), - .prepare = block_dirty_bitmap_disable_prepare, - .abort = block_dirty_bitmap_disable_abort, + .action = block_dirty_bitmap_disable_action, }, [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_MERGE] = { .instance_size = sizeof(BlockDirtyBitmapState), - .prepare = block_dirty_bitmap_merge_prepare, - .commit = block_dirty_bitmap_free_backup, - .abort = block_dirty_bitmap_restore, + .action = block_dirty_bitmap_merge_action, }, [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_REMOVE] = { .instance_size = sizeof(BlockDirtyBitmapState), - .prepare = block_dirty_bitmap_remove_prepare, - .commit = block_dirty_bitmap_remove_commit, - .abort = block_dirty_bitmap_remove_abort, + .action = block_dirty_bitmap_remove_action, }, /* Where are transactions for MIRROR, COMMIT and STREAM? * Although these blockjobs use transaction callbacks like the backup job, @@ -XXX,XX +XXX,XX @@ void qmp_transaction(TransactionActionList *dev_list, TransactionActionList *dev_entry = dev_list; bool has_props = !!props; JobTxn *block_job_txn = NULL; - BlkActionState *state, *next; Error *local_err = NULL; + Transaction *tran = tran_new(); GLOBAL_STATE_CODE(); - QTAILQ_HEAD(, BlkActionState) snap_bdrv_states; - QTAILQ_INIT(&snap_bdrv_states); - /* Does this transaction get canceled as a group on failure? * If not, we don't really need to make a JobTxn. */ @@ -XXX,XX +XXX,XX @@ void qmp_transaction(TransactionActionList *dev_list, while (NULL != dev_entry) { TransactionAction *dev_info = NULL; const BlkActionOps *ops; + BlkActionState *state; dev_info = dev_entry->value; dev_entry = dev_entry->next; @@ -XXX,XX +XXX,XX @@ void qmp_transaction(TransactionActionList *dev_list, state->action = dev_info; state->block_job_txn = block_job_txn; state->txn_props = props; - QTAILQ_INSERT_TAIL(&snap_bdrv_states, state, entry); - state->ops->prepare(state, &local_err); + state->ops->action(state, tran, &local_err); if (local_err) { error_propagate(errp, local_err); goto delete_and_fail; } } - QTAILQ_FOREACH(state, &snap_bdrv_states, entry) { - if (state->ops->commit) { - state->ops->commit(state); - } - } + tran_commit(tran); /* success */ goto exit; delete_and_fail: /* failure, and it is all-or-none; roll back all operations */ - QTAILQ_FOREACH_REVERSE(state, &snap_bdrv_states, entry) { - if (state->ops->abort) { - state->ops->abort(state); - } - } + tran_abort(tran); exit: - QTAILQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) { - if (state->ops->clean) { - state->ops->clean(state); - } - g_free(state); - } if (!has_props) { qapi_free_TransactionProperties(props); } -- 2.40.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Look at qmp_transaction(): dev_list is not obvious name for list of actions. Let's look at qapi spec, this argument is "actions". Let's follow the common practice of using same argument names in qapi scheme and code. To be honest, rename props to properties for same reason. Next, we have to rename global map of actions, to not conflict with new name for function argument. Rename also dev_entry loop variable accordingly to new name of the list. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510150624.310640-3-vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ static void abort_commit(void *opaque) g_assert_not_reached(); /* this action never succeeds */ } -static const BlkActionOps actions[] = { +static const BlkActionOps actions_map[] = { [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT] = { .instance_size = sizeof(ExternalSnapshotState), .action = external_snapshot_action, @@ -XXX,XX +XXX,XX @@ static TransactionProperties *get_transaction_properties( * * Always run under BQL. */ -void qmp_transaction(TransactionActionList *dev_list, - struct TransactionProperties *props, +void qmp_transaction(TransactionActionList *actions, + struct TransactionProperties *properties, Error **errp) { - TransactionActionList *dev_entry = dev_list; - bool has_props = !!props; + TransactionActionList *act = actions; + bool has_properties = !!properties; JobTxn *block_job_txn = NULL; Error *local_err = NULL; Transaction *tran = tran_new(); @@ -XXX,XX +XXX,XX @@ void qmp_transaction(TransactionActionList *dev_list, /* Does this transaction get canceled as a group on failure? * If not, we don't really need to make a JobTxn. */ - props = get_transaction_properties(props); - if (props->completion_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) { + properties = get_transaction_properties(properties); + if (properties->completion_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) { block_job_txn = job_txn_new(); } @@ -XXX,XX +XXX,XX @@ void qmp_transaction(TransactionActionList *dev_list, bdrv_drain_all(); /* We don't do anything in this loop that commits us to the operations */ - while (NULL != dev_entry) { + while (NULL != act) { TransactionAction *dev_info = NULL; const BlkActionOps *ops; BlkActionState *state; - dev_info = dev_entry->value; - dev_entry = dev_entry->next; + dev_info = act->value; + act = act->next; - assert(dev_info->type < ARRAY_SIZE(actions)); + assert(dev_info->type < ARRAY_SIZE(actions_map)); - ops = &actions[dev_info->type]; + ops = &actions_map[dev_info->type]; assert(ops->instance_size > 0); state = g_malloc0(ops->instance_size); state->ops = ops; state->action = dev_info; state->block_job_txn = block_job_txn; - state->txn_props = props; + state->txn_props = properties; state->ops->action(state, tran, &local_err); if (local_err) { @@ -XXX,XX +XXX,XX @@ delete_and_fail: /* failure, and it is all-or-none; roll back all operations */ tran_abort(tran); exit: - if (!has_props) { - qapi_free_TransactionProperties(props); + if (!has_properties) { + qapi_free_TransactionProperties(properties); } job_txn_unref(block_job_txn); } -- 2.40.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510150624.310640-4-vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ void qmp_transaction(TransactionActionList *actions, struct TransactionProperties *properties, Error **errp) { - TransactionActionList *act = actions; + TransactionActionList *act; bool has_properties = !!properties; JobTxn *block_job_txn = NULL; Error *local_err = NULL; @@ -XXX,XX +XXX,XX @@ void qmp_transaction(TransactionActionList *actions, bdrv_drain_all(); /* We don't do anything in this loop that commits us to the operations */ - while (NULL != act) { - TransactionAction *dev_info = NULL; + for (act = actions; act; act = act->next) { + TransactionAction *dev_info = act->value; const BlkActionOps *ops; BlkActionState *state; - dev_info = act->value; - act = act->next; - assert(dev_info->type < ARRAY_SIZE(actions_map)); ops = &actions_map[dev_info->type]; -- 2.40.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Only backup supports GROUPED mode. Make this logic more clear. And avoid passing extra thing to each action. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20230510150624.310640-5-vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 96 +++++++++++++----------------------------------------- 1 file changed, 22 insertions(+), 74 deletions(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ struct BlkActionState { TransactionAction *action; const BlkActionOps *ops; JobTxn *block_job_txn; - TransactionProperties *txn_props; QTAILQ_ENTRY(BlkActionState) entry; }; @@ -XXX,XX +XXX,XX @@ TransactionActionDrv internal_snapshot_drv = { .clean = internal_snapshot_clean, }; -static int action_check_completion_mode(BlkActionState *s, Error **errp) -{ - if (s->txn_props->completion_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) { - error_setg(errp, - "Action '%s' does not support Transaction property " - "completion-mode = %s", - TransactionActionKind_str(s->action->type), - ActionCompletionMode_str(s->txn_props->completion_mode)); - return -1; - } - return 0; -} - static void internal_snapshot_action(BlkActionState *common, Transaction *tran, Error **errp) { @@ -XXX,XX +XXX,XX @@ static void internal_snapshot_action(BlkActionState *common, tran_add(tran, &internal_snapshot_drv, state); - /* 1. parse input */ device = internal->device; name = internal->name; - /* 2. check for validation */ - if (action_check_completion_mode(common, errp) < 0) { - return; - } - bs = qmp_get_root_bs(device, errp); if (!bs) { return; @@ -XXX,XX +XXX,XX @@ static void external_snapshot_action(BlkActionState *common, Transaction *tran, } /* start processing */ - if (action_check_completion_mode(common, errp) < 0) { - return; - } state->old_bs = bdrv_lookup_bs(device, node_name, errp); if (!state->old_bs) { @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_add_action(BlkActionState *common, tran_add(tran, &block_dirty_bitmap_add_drv, state); - if (action_check_completion_mode(common, errp) < 0) { - return; - } - action = common->action->u.block_dirty_bitmap_add.data; /* AIO context taken and released within qmp_block_dirty_bitmap_add */ qmp_block_dirty_bitmap_add(action->node, action->name, @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_clear_action(BlkActionState *common, tran_add(tran, &block_dirty_bitmap_clear_drv, state); - if (action_check_completion_mode(common, errp) < 0) { - return; - } - action = common->action->u.block_dirty_bitmap_clear.data; state->bitmap = block_dirty_bitmap_lookup(action->node, action->name, @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_enable_action(BlkActionState *common, tran_add(tran, &block_dirty_bitmap_enable_drv, state); - if (action_check_completion_mode(common, errp) < 0) { - return; - } - action = common->action->u.block_dirty_bitmap_enable.data; state->bitmap = block_dirty_bitmap_lookup(action->node, action->name, @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_disable_action(BlkActionState *common, tran_add(tran, &block_dirty_bitmap_disable_drv, state); - if (action_check_completion_mode(common, errp) < 0) { - return; - } - action = common->action->u.block_dirty_bitmap_disable.data; state->bitmap = block_dirty_bitmap_lookup(action->node, action->name, @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_merge_action(BlkActionState *common, tran_add(tran, &block_dirty_bitmap_merge_drv, state); - if (action_check_completion_mode(common, errp) < 0) { - return; - } - action = common->action->u.block_dirty_bitmap_merge.data; state->bitmap = block_dirty_bitmap_merge(action->node, action->target, @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_remove_action(BlkActionState *common, tran_add(tran, &block_dirty_bitmap_remove_drv, state); - if (action_check_completion_mode(common, errp) < 0) { - return; - } - action = common->action->u.block_dirty_bitmap_remove.data; state->bitmap = block_dirty_bitmap_remove(action->node, action->name, @@ -XXX,XX +XXX,XX @@ static const BlkActionOps actions_map[] = { */ }; -/** - * Allocate a TransactionProperties structure if necessary, and fill - * that structure with desired defaults if they are unset. - */ -static TransactionProperties *get_transaction_properties( - TransactionProperties *props) -{ - if (!props) { - props = g_new0(TransactionProperties, 1); - } - - if (!props->has_completion_mode) { - props->has_completion_mode = true; - props->completion_mode = ACTION_COMPLETION_MODE_INDIVIDUAL; - } - - return props; -} - /* * 'Atomic' group operations. The operations are performed as a set, and if * any fail then we roll back all operations in the group. @@ -XXX,XX +XXX,XX @@ void qmp_transaction(TransactionActionList *actions, Error **errp) { TransactionActionList *act; - bool has_properties = !!properties; JobTxn *block_job_txn = NULL; Error *local_err = NULL; - Transaction *tran = tran_new(); + Transaction *tran; + ActionCompletionMode comp_mode = + properties ? properties->completion_mode : + ACTION_COMPLETION_MODE_INDIVIDUAL; GLOBAL_STATE_CODE(); /* Does this transaction get canceled as a group on failure? * If not, we don't really need to make a JobTxn. */ - properties = get_transaction_properties(properties); - if (properties->completion_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) { + if (comp_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) { + for (act = actions; act; act = act->next) { + TransactionActionKind type = act->value->type; + + if (type != TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP && + type != TRANSACTION_ACTION_KIND_DRIVE_BACKUP) + { + error_setg(errp, + "Action '%s' does not support transaction property " + "completion-mode = %s", + TransactionActionKind_str(type), + ActionCompletionMode_str(comp_mode)); + return; + } + } + block_job_txn = job_txn_new(); } /* drain all i/o before any operations */ bdrv_drain_all(); + tran = tran_new(); + /* We don't do anything in this loop that commits us to the operations */ for (act = actions; act; act = act->next) { TransactionAction *dev_info = act->value; @@ -XXX,XX +XXX,XX @@ void qmp_transaction(TransactionActionList *actions, state->ops = ops; state->action = dev_info; state->block_job_txn = block_job_txn; - state->txn_props = properties; state->ops->action(state, tran, &local_err); if (local_err) { @@ -XXX,XX +XXX,XX @@ delete_and_fail: /* failure, and it is all-or-none; roll back all operations */ tran_abort(tran); exit: - if (!has_properties) { - qapi_free_TransactionProperties(properties); - } job_txn_unref(block_job_txn); } -- 2.40.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Other bitmap related actions use the .bitmap pointer in .abort action, let's do same here: 1. It helps further refactoring, as bitmap-add is the only bitmap action that uses state.action in .abort 2. It must be safe: transaction actions rely on the fact that on .abort() the state is the same as at the end of .prepare(), so that in .abort() we could precisely rollback the changes done by .prepare(). The only way to remove the bitmap during transaction should be block-dirty-bitmap-remove action, but it postpones actual removal to .commit(), so we are OK on any rollback path. (Note also that bitmap-remove is the only bitmap action that has .commit() phase, except for simple g_free the state on .clean()) 3. Again, other bitmap actions behave this way: keep the bitmap pointer during the transaction. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20230510150624.310640-6-vsementsov@yandex-team.ru> [kwolf: Also remove the now unused BlockDirtyBitmapState.prepared] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ typedef struct BlockDirtyBitmapState { BdrvDirtyBitmap *bitmap; BlockDriverState *bs; HBitmap *backup; - bool prepared; bool was_enabled; } BlockDirtyBitmapState; @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_add_action(BlkActionState *common, &local_err); if (!local_err) { - state->prepared = true; + state->bitmap = block_dirty_bitmap_lookup(action->node, action->name, + NULL, &error_abort); } else { error_propagate(errp, local_err); } @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_add_action(BlkActionState *common, static void block_dirty_bitmap_add_abort(void *opaque) { - BlockDirtyBitmapAdd *action; BlockDirtyBitmapState *state = opaque; - action = state->common.action->u.block_dirty_bitmap_add.data; - /* Should not be able to fail: IF the bitmap was added via .prepare(), - * then the node reference and bitmap name must have been valid. - */ - if (state->prepared) { - qmp_block_dirty_bitmap_remove(action->node, action->name, &error_abort); + if (state->bitmap) { + bdrv_release_dirty_bitmap(state->bitmap); } } -- 2.40.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Let's simplify things: First, actions generally don't need access to common BlkActionState structure. The only exclusion are backup actions that need block_job_txn. Next, for transaction actions of Transaction API is more native to allocated state structure in the action itself. So, do the following transformation: 1. Let all actions be represented by a function with corresponding structure as arguments. 2. Instead of array-map marshaller, let's make a function, that calls corresponding action directly. 3. BlkActionOps and BlkActionState structures become unused Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20230510150624.310640-7-vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 265 +++++++++++++++++------------------------------------ 1 file changed, 85 insertions(+), 180 deletions(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ out_aio_context: return NULL; } -/* New and old BlockDriverState structs for atomic group operations */ - -typedef struct BlkActionState BlkActionState; - -/** - * BlkActionOps: - * Table of operations that define an Action. - * - * @instance_size: Size of state struct, in bytes. - * @prepare: Prepare the work, must NOT be NULL. - * @commit: Commit the changes, can be NULL. - * @abort: Abort the changes on fail, can be NULL. - * @clean: Clean up resources after all transaction actions have called - * commit() or abort(). Can be NULL. - * - * Only prepare() may fail. In a single transaction, only one of commit() or - * abort() will be called. clean() will always be called if it is present. - * - * Always run under BQL. - */ -typedef struct BlkActionOps { - size_t instance_size; - void (*action)(BlkActionState *common, Transaction *tran, Error **errp); -} BlkActionOps; - -/** - * BlkActionState: - * Describes one Action's state within a Transaction. - * - * @action: QAPI-defined enum identifying which Action to perform. - * @ops: Table of ActionOps this Action can perform. - * @block_job_txn: Transaction which this action belongs to. - * @entry: List membership for all Actions in this Transaction. - * - * This structure must be arranged as first member in a subclassed type, - * assuming that the compiler will also arrange it to the same offsets as the - * base class. - */ -struct BlkActionState { - TransactionAction *action; - const BlkActionOps *ops; - JobTxn *block_job_txn; - QTAILQ_ENTRY(BlkActionState) entry; -}; - /* internal snapshot private data */ typedef struct InternalSnapshotState { - BlkActionState common; BlockDriverState *bs; QEMUSnapshotInfo sn; bool created; @@ -XXX,XX +XXX,XX @@ TransactionActionDrv internal_snapshot_drv = { .clean = internal_snapshot_clean, }; -static void internal_snapshot_action(BlkActionState *common, +static void internal_snapshot_action(BlockdevSnapshotInternal *internal, Transaction *tran, Error **errp) { Error *local_err = NULL; @@ -XXX,XX +XXX,XX @@ static void internal_snapshot_action(BlkActionState *common, QEMUSnapshotInfo old_sn, *sn; bool ret; int64_t rt; - BlockdevSnapshotInternal *internal; - InternalSnapshotState *state; + InternalSnapshotState *state = g_new0(InternalSnapshotState, 1); AioContext *aio_context; int ret1; - g_assert(common->action->type == - TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC); - internal = common->action->u.blockdev_snapshot_internal_sync.data; - state = DO_UPCAST(InternalSnapshotState, common, common); - tran_add(tran, &internal_snapshot_drv, state); device = internal->device; @@ -XXX,XX +XXX,XX @@ static void internal_snapshot_clean(void *opaque) /* external snapshot private data */ typedef struct ExternalSnapshotState { - BlkActionState common; BlockDriverState *old_bs; BlockDriverState *new_bs; bool overlay_appended; @@ -XXX,XX +XXX,XX @@ TransactionActionDrv external_snapshot_drv = { .clean = external_snapshot_clean, }; -static void external_snapshot_action(BlkActionState *common, Transaction *tran, - Error **errp) +static void external_snapshot_action(TransactionAction *action, + Transaction *tran, Error **errp) { int ret; int flags = 0; @@ -XXX,XX +XXX,XX @@ static void external_snapshot_action(BlkActionState *common, Transaction *tran, const char *snapshot_ref; /* File name of the new image (for 'blockdev-snapshot-sync') */ const char *new_image_file; - ExternalSnapshotState *state = - DO_UPCAST(ExternalSnapshotState, common, common); - TransactionAction *action = common->action; + ExternalSnapshotState *state = g_new0(ExternalSnapshotState, 1); AioContext *aio_context; uint64_t perm, shared; @@ -XXX,XX +XXX,XX @@ static void external_snapshot_clean(void *opaque) } typedef struct DriveBackupState { - BlkActionState common; BlockDriverState *bs; BlockJob *job; } DriveBackupState; @@ -XXX,XX +XXX,XX @@ TransactionActionDrv drive_backup_drv = { .clean = drive_backup_clean, }; -static void drive_backup_action(BlkActionState *common, Transaction *tran, - Error **errp) +static void drive_backup_action(DriveBackup *backup, + JobTxn *block_job_txn, + Transaction *tran, Error **errp) { - DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); - DriveBackup *backup; + DriveBackupState *state = g_new0(DriveBackupState, 1); BlockDriverState *bs; BlockDriverState *target_bs; BlockDriverState *source = NULL; @@ -XXX,XX +XXX,XX @@ static void drive_backup_action(BlkActionState *common, Transaction *tran, tran_add(tran, &drive_backup_drv, state); - assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP); - backup = common->action->u.drive_backup.data; - if (!backup->has_mode) { backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; } @@ -XXX,XX +XXX,XX @@ static void drive_backup_action(BlkActionState *common, Transaction *tran, state->job = do_backup_common(qapi_DriveBackup_base(backup), bs, target_bs, aio_context, - common->block_job_txn, errp); + block_job_txn, errp); unref: bdrv_unref(target_bs); @@ -XXX,XX +XXX,XX @@ static void drive_backup_clean(void *opaque) } typedef struct BlockdevBackupState { - BlkActionState common; BlockDriverState *bs; BlockJob *job; } BlockdevBackupState; @@ -XXX,XX +XXX,XX @@ TransactionActionDrv blockdev_backup_drv = { .clean = blockdev_backup_clean, }; -static void blockdev_backup_action(BlkActionState *common, Transaction *tran, - Error **errp) +static void blockdev_backup_action(BlockdevBackup *backup, + JobTxn *block_job_txn, + Transaction *tran, Error **errp) { - BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); - BlockdevBackup *backup; + BlockdevBackupState *state = g_new0(BlockdevBackupState, 1); BlockDriverState *bs; BlockDriverState *target_bs; AioContext *aio_context; @@ -XXX,XX +XXX,XX @@ static void blockdev_backup_action(BlkActionState *common, Transaction *tran, tran_add(tran, &blockdev_backup_drv, state); - assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP); - backup = common->action->u.blockdev_backup.data; - bs = bdrv_lookup_bs(backup->device, backup->device, errp); if (!bs) { return; @@ -XXX,XX +XXX,XX @@ static void blockdev_backup_action(BlkActionState *common, Transaction *tran, state->job = do_backup_common(qapi_BlockdevBackup_base(backup), bs, target_bs, aio_context, - common->block_job_txn, errp); + block_job_txn, errp); aio_context_release(aio_context); } @@ -XXX,XX +XXX,XX @@ static void blockdev_backup_clean(void *opaque) } typedef struct BlockDirtyBitmapState { - BlkActionState common; BdrvDirtyBitmap *bitmap; BlockDriverState *bs; HBitmap *backup; @@ -XXX,XX +XXX,XX @@ TransactionActionDrv block_dirty_bitmap_add_drv = { .clean = g_free, }; -static void block_dirty_bitmap_add_action(BlkActionState *common, +static void block_dirty_bitmap_add_action(BlockDirtyBitmapAdd *action, Transaction *tran, Error **errp) { Error *local_err = NULL; - BlockDirtyBitmapAdd *action; - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); + BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1); tran_add(tran, &block_dirty_bitmap_add_drv, state); - action = common->action->u.block_dirty_bitmap_add.data; /* AIO context taken and released within qmp_block_dirty_bitmap_add */ qmp_block_dirty_bitmap_add(action->node, action->name, action->has_granularity, action->granularity, @@ -XXX,XX +XXX,XX @@ TransactionActionDrv block_dirty_bitmap_clear_drv = { .clean = g_free, }; -static void block_dirty_bitmap_clear_action(BlkActionState *common, +static void block_dirty_bitmap_clear_action(BlockDirtyBitmap *action, Transaction *tran, Error **errp) { - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); - BlockDirtyBitmap *action; + BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1); tran_add(tran, &block_dirty_bitmap_clear_drv, state); - action = common->action->u.block_dirty_bitmap_clear.data; state->bitmap = block_dirty_bitmap_lookup(action->node, action->name, &state->bs, @@ -XXX,XX +XXX,XX @@ TransactionActionDrv block_dirty_bitmap_enable_drv = { .clean = g_free, }; -static void block_dirty_bitmap_enable_action(BlkActionState *common, +static void block_dirty_bitmap_enable_action(BlockDirtyBitmap *action, Transaction *tran, Error **errp) { - BlockDirtyBitmap *action; - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); + BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1); tran_add(tran, &block_dirty_bitmap_enable_drv, state); - action = common->action->u.block_dirty_bitmap_enable.data; state->bitmap = block_dirty_bitmap_lookup(action->node, action->name, NULL, @@ -XXX,XX +XXX,XX @@ TransactionActionDrv block_dirty_bitmap_disable_drv = { .clean = g_free, }; -static void block_dirty_bitmap_disable_action(BlkActionState *common, +static void block_dirty_bitmap_disable_action(BlockDirtyBitmap *action, Transaction *tran, Error **errp) { - BlockDirtyBitmap *action; - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); + BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1); tran_add(tran, &block_dirty_bitmap_disable_drv, state); - action = common->action->u.block_dirty_bitmap_disable.data; state->bitmap = block_dirty_bitmap_lookup(action->node, action->name, NULL, @@ -XXX,XX +XXX,XX @@ TransactionActionDrv block_dirty_bitmap_merge_drv = { .clean = g_free, }; -static void block_dirty_bitmap_merge_action(BlkActionState *common, +static void block_dirty_bitmap_merge_action(BlockDirtyBitmapMerge *action, Transaction *tran, Error **errp) { - BlockDirtyBitmapMerge *action; - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); + BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1); tran_add(tran, &block_dirty_bitmap_merge_drv, state); - action = common->action->u.block_dirty_bitmap_merge.data; - state->bitmap = block_dirty_bitmap_merge(action->node, action->target, action->bitmaps, &state->backup, errp); @@ -XXX,XX +XXX,XX @@ TransactionActionDrv block_dirty_bitmap_remove_drv = { .clean = g_free, }; -static void block_dirty_bitmap_remove_action(BlkActionState *common, +static void block_dirty_bitmap_remove_action(BlockDirtyBitmap *action, Transaction *tran, Error **errp) { - BlockDirtyBitmap *action; - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); + BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1); tran_add(tran, &block_dirty_bitmap_remove_drv, state); - action = common->action->u.block_dirty_bitmap_remove.data; state->bitmap = block_dirty_bitmap_remove(action->node, action->name, false, &state->bs, errp); @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_remove_commit(void *opaque) static void abort_commit(void *opaque); TransactionActionDrv abort_drv = { .commit = abort_commit, - .clean = g_free, }; -static void abort_action(BlkActionState *common, Transaction *tran, - Error **errp) +static void abort_action(Transaction *tran, Error **errp) { - tran_add(tran, &abort_drv, common); + tran_add(tran, &abort_drv, NULL); error_setg(errp, "Transaction aborted using Abort action"); } @@ -XXX,XX +XXX,XX @@ static void abort_commit(void *opaque) g_assert_not_reached(); /* this action never succeeds */ } -static const BlkActionOps actions_map[] = { - [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT] = { - .instance_size = sizeof(ExternalSnapshotState), - .action = external_snapshot_action, - }, - [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = { - .instance_size = sizeof(ExternalSnapshotState), - .action = external_snapshot_action, - }, - [TRANSACTION_ACTION_KIND_DRIVE_BACKUP] = { - .instance_size = sizeof(DriveBackupState), - .action = drive_backup_action, - }, - [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = { - .instance_size = sizeof(BlockdevBackupState), - .action = blockdev_backup_action, - }, - [TRANSACTION_ACTION_KIND_ABORT] = { - .instance_size = sizeof(BlkActionState), - .action = abort_action, - }, - [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC] = { - .instance_size = sizeof(InternalSnapshotState), - .action = internal_snapshot_action, - }, - [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = { - .instance_size = sizeof(BlockDirtyBitmapState), - .action = block_dirty_bitmap_add_action, - }, - [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = { - .instance_size = sizeof(BlockDirtyBitmapState), - .action = block_dirty_bitmap_clear_action, - }, - [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = { - .instance_size = sizeof(BlockDirtyBitmapState), - .action = block_dirty_bitmap_enable_action, - }, - [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = { - .instance_size = sizeof(BlockDirtyBitmapState), - .action = block_dirty_bitmap_disable_action, - }, - [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_MERGE] = { - .instance_size = sizeof(BlockDirtyBitmapState), - .action = block_dirty_bitmap_merge_action, - }, - [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_REMOVE] = { - .instance_size = sizeof(BlockDirtyBitmapState), - .action = block_dirty_bitmap_remove_action, - }, - /* Where are transactions for MIRROR, COMMIT and STREAM? +static void transaction_action(TransactionAction *act, JobTxn *block_job_txn, + Transaction *tran, Error **errp) +{ + switch (act->type) { + case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT: + case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC: + external_snapshot_action(act, tran, errp); + return; + case TRANSACTION_ACTION_KIND_DRIVE_BACKUP: + drive_backup_action(act->u.drive_backup.data, + block_job_txn, tran, errp); + return; + case TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP: + blockdev_backup_action(act->u.blockdev_backup.data, + block_job_txn, tran, errp); + return; + case TRANSACTION_ACTION_KIND_ABORT: + abort_action(tran, errp); + return; + case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC: + internal_snapshot_action(act->u.blockdev_snapshot_internal_sync.data, + tran, errp); + return; + case TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD: + block_dirty_bitmap_add_action(act->u.block_dirty_bitmap_add.data, + tran, errp); + return; + case TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR: + block_dirty_bitmap_clear_action(act->u.block_dirty_bitmap_clear.data, + tran, errp); + return; + case TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE: + block_dirty_bitmap_enable_action(act->u.block_dirty_bitmap_enable.data, + tran, errp); + return; + case TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE: + block_dirty_bitmap_disable_action( + act->u.block_dirty_bitmap_disable.data, tran, errp); + return; + case TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_MERGE: + block_dirty_bitmap_merge_action(act->u.block_dirty_bitmap_merge.data, + tran, errp); + return; + case TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_REMOVE: + block_dirty_bitmap_remove_action(act->u.block_dirty_bitmap_remove.data, + tran, errp); + return; + /* + * Where are transactions for MIRROR, COMMIT and STREAM? * Although these blockjobs use transaction callbacks like the backup job, * these jobs do not necessarily adhere to transaction semantics. * These jobs may not fully undo all of their actions on abort, nor do they * necessarily work in transactions with more than one job in them. */ -}; + case TRANSACTION_ACTION_KIND__MAX: + default: + g_assert_not_reached(); + }; +} + /* * 'Atomic' group operations. The operations are performed as a set, and if @@ -XXX,XX +XXX,XX @@ void qmp_transaction(TransactionActionList *actions, /* We don't do anything in this loop that commits us to the operations */ for (act = actions; act; act = act->next) { - TransactionAction *dev_info = act->value; - const BlkActionOps *ops; - BlkActionState *state; - - assert(dev_info->type < ARRAY_SIZE(actions_map)); - - ops = &actions_map[dev_info->type]; - assert(ops->instance_size > 0); - - state = g_malloc0(ops->instance_size); - state->ops = ops; - state->action = dev_info; - state->block_job_txn = block_job_txn; - - state->ops->action(state, tran, &local_err); + transaction_action(act->value, block_job_txn, tran, &local_err); if (local_err) { error_propagate(errp, local_err); goto delete_and_fail; -- 2.40.1 From: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp> "zlib" clusters are actually raw deflate (RFC1951) clusters without zlib headers. Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp> Message-Id: <168424874322.11954.1340942046351859521-0@git.sr.ht> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- docs/interop/qcow2.txt | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt index XXXXXXX..XXXXXXX 100644 --- a/docs/interop/qcow2.txt +++ b/docs/interop/qcow2.txt @@ -XXX,XX +XXX,XX @@ version 2. type. If the incompatible bit "Compression type" is set: the field - must be present and non-zero (which means non-zlib + must be present and non-zero (which means non-deflate compression type). Otherwise, this field must not be present - or must be zero (which means zlib). + or must be zero (which means deflate). Available compression type values: - 0: zlib <https://www.zlib.net/> + 0: deflate <https://www.ietf.org/rfc/rfc1951.txt> 1: zstd <http://github.com/facebook/zstd> + The deflate compression type is called "zlib" + <https://www.zlib.net/> in QEMU. However, clusters with the + deflate compression type do not have zlib headers. + === Header padding === -- 2.40.1 These are functions that modify the graph, so they must be able to take a writer lock. This is impossible if they already hold the reader lock. If they need a reader lock for some of their operations, they should take it internally. Many of them go through blk_*(), which will always take the lock itself. Direct calls of bdrv_*() need to take the reader lock. Note that while locking for bdrv_co_*() calls is checked by TSA, this is not the case for the mixed_coroutine_fns bdrv_*(). Holding the lock is still required when they are called from coroutine context like here! This effectively reverts 4ec8df0183, but adds some internal locking instead. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-2-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/block-global-state.h | 8 +++---- include/block/block_int-common.h | 4 ++-- block.c | 1 - block/create.c | 1 - block/crypto.c | 25 ++++++++++---------- block/parallels.c | 6 ++--- block/qcow.c | 6 ++--- block/qcow2.c | 37 +++++++++++++++++++----------- block/qed.c | 6 ++--- block/raw-format.c | 2 +- block/vdi.c | 11 +++++---- block/vhdx.c | 8 ++++--- block/vmdk.c | 27 ++++++++++------------ block/vpc.c | 6 ++--- 14 files changed, 78 insertions(+), 70 deletions(-) diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -XXX,XX +XXX,XX @@ BlockDriver *bdrv_find_protocol(const char *filename, Error **errp); BlockDriver *bdrv_find_format(const char *format_name); -int coroutine_fn GRAPH_RDLOCK +int coroutine_fn GRAPH_UNLOCKED bdrv_co_create(BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp); -int co_wrapper_bdrv_rdlock bdrv_create(BlockDriver *drv, const char *filename, - QemuOpts *opts, Error **errp); +int co_wrapper bdrv_create(BlockDriver *drv, const char *filename, + QemuOpts *opts, Error **errp); -int coroutine_fn GRAPH_RDLOCK +int coroutine_fn GRAPH_UNLOCKED bdrv_co_create_file(const char *filename, QemuOpts *opts, Error **errp); BlockDriverState *bdrv_new(void); diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -XXX,XX +XXX,XX @@ struct BlockDriver { BlockDriverState *bs, QDict *options, int flags, Error **errp); void (*bdrv_close)(BlockDriverState *bs); - int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_create)( + int coroutine_fn GRAPH_UNLOCKED_PTR (*bdrv_co_create)( BlockdevCreateOptions *opts, Error **errp); - int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_create_opts)( + int coroutine_fn GRAPH_UNLOCKED_PTR (*bdrv_co_create_opts)( BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp); int (*bdrv_amend_options)(BlockDriverState *bs, diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename, int ret; GLOBAL_STATE_CODE(); ERRP_GUARD(); - assert_bdrv_graph_readable(); if (!drv->bdrv_co_create_opts) { error_setg(errp, "Driver '%s' does not support image creation", diff --git a/block/create.c b/block/create.c index XXXXXXX..XXXXXXX 100644 --- a/block/create.c +++ b/block/create.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn blockdev_create_run(Job *job, Error **errp) int ret; GLOBAL_STATE_CODE(); - GRAPH_RDLOCK_GUARD(); job_progress_set_remaining(&s->common, 1); ret = s->drv->bdrv_co_create(s->opts, errp); diff --git a/block/crypto.c b/block/crypto.c index XXXXXXX..XXXXXXX 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -XXX,XX +XXX,XX @@ struct BlockCryptoCreateData { }; -static int block_crypto_create_write_func(QCryptoBlock *block, - size_t offset, - const uint8_t *buf, - size_t buflen, - void *opaque, - Error **errp) +static int coroutine_fn GRAPH_UNLOCKED +block_crypto_create_write_func(QCryptoBlock *block, size_t offset, + const uint8_t *buf, size_t buflen, void *opaque, + Error **errp) { struct BlockCryptoCreateData *data = opaque; ssize_t ret; @@ -XXX,XX +XXX,XX @@ static int block_crypto_create_write_func(QCryptoBlock *block, return 0; } -static int block_crypto_create_init_func(QCryptoBlock *block, - size_t headerlen, - void *opaque, - Error **errp) +static int coroutine_fn GRAPH_UNLOCKED +block_crypto_create_init_func(QCryptoBlock *block, size_t headerlen, + void *opaque, Error **errp) { struct BlockCryptoCreateData *data = opaque; Error *local_error = NULL; @@ -XXX,XX +XXX,XX @@ static int block_crypto_open_generic(QCryptoBlockFormat format, } -static int coroutine_fn +static int coroutine_fn GRAPH_UNLOCKED block_crypto_co_create_generic(BlockDriverState *bs, int64_t size, QCryptoBlockCreateOptions *opts, PreallocMode prealloc, Error **errp) @@ -XXX,XX +XXX,XX @@ static int block_crypto_open_luks(BlockDriverState *bs, bs, options, flags, errp); } -static int coroutine_fn +static int coroutine_fn GRAPH_UNLOCKED block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) { BlockdevCreateOptionsLUKS *luks_opts; @@ -XXX,XX +XXX,XX @@ fail: return ret; } -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED block_crypto_co_create_opts_luks(BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp) { @@ -XXX,XX +XXX,XX @@ fail: * beforehand, it has been truncated and corrupted in the process. */ if (ret) { + bdrv_graph_co_rdlock(); bdrv_co_delete_file_noerr(bs); + bdrv_graph_co_rdunlock(); } bdrv_co_unref(bs); diff --git a/block/parallels.c b/block/parallels.c index XXXXXXX..XXXXXXX 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -XXX,XX +XXX,XX @@ out: } -static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts, - Error **errp) +static int coroutine_fn GRAPH_UNLOCKED +parallels_co_create(BlockdevCreateOptions* opts, Error **errp) { BlockdevCreateOptionsParallels *parallels_opts; BlockDriverState *bs; @@ -XXX,XX +XXX,XX @@ exit: goto out; } -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED parallels_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp) { diff --git a/block/qcow.c b/block/qcow.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -XXX,XX +XXX,XX @@ static void qcow_close(BlockDriverState *bs) error_free(s->migration_blocker); } -static int coroutine_fn qcow_co_create(BlockdevCreateOptions *opts, - Error **errp) +static int coroutine_fn GRAPH_UNLOCKED +qcow_co_create(BlockdevCreateOptions *opts, Error **errp) { BlockdevCreateOptionsQcow *qcow_opts; int header_size, backing_filename_len, l1_size, shift, i; @@ -XXX,XX +XXX,XX @@ exit: return ret; } -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED qcow_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp) { diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ static int qcow2_crypto_hdr_read_func(QCryptoBlock *block, size_t offset, } -static int qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t headerlen, - void *opaque, Error **errp) +static int coroutine_fn GRAPH_RDLOCK +qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t headerlen, void *opaque, + Error **errp) { BlockDriverState *bs = opaque; BDRVQcow2State *s = bs->opaque; @@ -XXX,XX +XXX,XX @@ static int qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t headerlen, */ clusterlen = size_to_clusters(s, headerlen) * s->cluster_size; assert(qcow2_pre_write_overlap_check(bs, 0, ret, clusterlen, false) == 0); - ret = bdrv_pwrite_zeroes(bs->file, - ret, - clusterlen, 0); + ret = bdrv_co_pwrite_zeroes(bs->file, ret, clusterlen, 0); if (ret < 0) { error_setg_errno(errp, -ret, "Could not zero fill encryption header"); return -1; @@ -XXX,XX +XXX,XX @@ static int qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t headerlen, } -static int qcow2_crypto_hdr_write_func(QCryptoBlock *block, size_t offset, - const uint8_t *buf, size_t buflen, - void *opaque, Error **errp) +/* The graph lock must be held when called in coroutine context */ +static int coroutine_mixed_fn +qcow2_crypto_hdr_write_func(QCryptoBlock *block, size_t offset, + const uint8_t *buf, size_t buflen, + void *opaque, Error **errp) { BlockDriverState *bs = opaque; BDRVQcow2State *s = bs->opaque; @@ -XXX,XX +XXX,XX @@ static int qcow2_change_backing_file(BlockDriverState *bs, return qcow2_update_header(bs); } -static int qcow2_set_up_encryption(BlockDriverState *bs, - QCryptoBlockCreateOptions *cryptoopts, - Error **errp) +static int coroutine_fn GRAPH_RDLOCK +qcow2_set_up_encryption(BlockDriverState *bs, + QCryptoBlockCreateOptions *cryptoopts, + Error **errp) { BDRVQcow2State *s = bs->opaque; QCryptoBlock *crypto = NULL; @@ -XXX,XX +XXX,XX @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version, return refcount_bits; } -static int coroutine_fn +static int coroutine_fn GRAPH_UNLOCKED qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) { BlockdevCreateOptionsQcow2 *qcow2_opts; @@ -XXX,XX +XXX,XX @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) goto out; } + bdrv_graph_co_rdlock(); ret = qcow2_alloc_clusters(blk_bs(blk), 3 * cluster_size); if (ret < 0) { + bdrv_graph_co_rdunlock(); error_setg_errno(errp, -ret, "Could not allocate clusters for qcow2 " "header and refcount table"); goto out; @@ -XXX,XX +XXX,XX @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) /* Create a full header (including things like feature table) */ ret = qcow2_update_header(blk_bs(blk)); + bdrv_graph_co_rdunlock(); + if (ret < 0) { error_setg_errno(errp, -ret, "Could not update qcow2 header"); goto out; @@ -XXX,XX +XXX,XX @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) /* Want encryption? There you go. */ if (qcow2_opts->encrypt) { + bdrv_graph_co_rdlock(); ret = qcow2_set_up_encryption(blk_bs(blk), qcow2_opts->encrypt, errp); + bdrv_graph_co_rdunlock(); + if (ret < 0) { goto out; } @@ -XXX,XX +XXX,XX @@ out: return ret; } -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp) { @@ -XXX,XX +XXX,XX @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts, ret = qcow2_co_create(create_options, errp); finish: if (ret < 0) { + bdrv_graph_co_rdlock(); bdrv_co_delete_file_noerr(bs); bdrv_co_delete_file_noerr(data_bs); + bdrv_graph_co_rdunlock(); } else { ret = 0; } diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static void bdrv_qed_close(BlockDriverState *bs) qemu_vfree(s->l1_table); } -static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts, - Error **errp) +static int coroutine_fn GRAPH_UNLOCKED +bdrv_qed_co_create(BlockdevCreateOptions *opts, Error **errp) { BlockdevCreateOptionsQed *qed_opts; BlockBackend *blk = NULL; @@ -XXX,XX +XXX,XX @@ out: return ret; } -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED bdrv_qed_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp) { diff --git a/block/raw-format.c b/block/raw-format.c index XXXXXXX..XXXXXXX 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -XXX,XX +XXX,XX @@ static int raw_has_zero_init(BlockDriverState *bs) return bdrv_has_zero_init(bs->file->bs); } -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED raw_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp) { diff --git a/block/vdi.c b/block/vdi.c index XXXXXXX..XXXXXXX 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -XXX,XX +XXX,XX @@ nonallocating_write: return ret; } -static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options, - size_t block_size, Error **errp) +static int coroutine_fn GRAPH_UNLOCKED +vdi_co_do_create(BlockdevCreateOptions *create_options, size_t block_size, + Error **errp) { BlockdevCreateOptionsVdi *vdi_opts; int ret = 0; @@ -XXX,XX +XXX,XX @@ exit: return ret; } -static int coroutine_fn vdi_co_create(BlockdevCreateOptions *create_options, - Error **errp) +static int coroutine_fn GRAPH_UNLOCKED +vdi_co_create(BlockdevCreateOptions *create_options, Error **errp) { return vdi_co_do_create(create_options, DEFAULT_CLUSTER_SIZE, errp); } -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED vdi_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp) { diff --git a/block/vhdx.c b/block/vhdx.c index XXXXXXX..XXXXXXX 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -XXX,XX +XXX,XX @@ exit: * There are 2 headers, and the highest sequence number will represent * the active header */ -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED vhdx_create_new_headers(BlockBackend *blk, uint64_t image_size, uint32_t log_size) { @@ -XXX,XX +XXX,XX @@ vhdx_create_new_headers(BlockBackend *blk, uint64_t image_size, int ret = 0; VHDXHeader *hdr = NULL; + GRAPH_RDLOCK_GUARD(); + hdr = g_new0(VHDXHeader, 1); hdr->signature = VHDX_HEADER_SIGNATURE; @@ -XXX,XX +XXX,XX @@ exit: * .---- ~ ----------- ~ ------------ ~ ---------------- ~ -----------. * 1MB */ -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED vhdx_co_create(BlockdevCreateOptions *opts, Error **errp) { BlockdevCreateOptionsVhdx *vhdx_opts; @@ -XXX,XX +XXX,XX @@ delete_and_exit: return ret; } -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED vhdx_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp) { diff --git a/block/vmdk.c b/block/vmdk.c index XXXXXXX..XXXXXXX 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -XXX,XX +XXX,XX @@ vmdk_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes, return ret; } -static int vmdk_init_extent(BlockBackend *blk, - int64_t filesize, bool flat, - bool compress, bool zeroed_grain, - Error **errp) +static int GRAPH_UNLOCKED +vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress, + bool zeroed_grain, Error **errp) { int ret, i; VMDK4Header header; @@ -XXX,XX +XXX,XX @@ exit: return ret; } -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED vmdk_create_extent(const char *filename, int64_t filesize, bool flat, bool compress, bool zeroed_grain, BlockBackend **pbb, QemuOpts *opts, Error **errp) @@ -XXX,XX +XXX,XX @@ static int filename_decompose(const char *filename, char *path, char *prefix, * non-split format. * idx >= 1: get the n-th extent if in a split subformat */ -typedef BlockBackend * coroutine_fn /* GRAPH_RDLOCK */ +typedef BlockBackend * coroutine_fn GRAPH_UNLOCKED_PTR (*vmdk_create_extent_fn)(int64_t size, int idx, bool flat, bool split, bool compress, bool zeroed_grain, void *opaque, Error **errp); @@ -XXX,XX +XXX,XX @@ static void vmdk_desc_add_extent(GString *desc, g_free(basename); } -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED vmdk_co_do_create(int64_t size, BlockdevVmdkSubformat subformat, BlockdevVmdkAdapterType adapter_type, @@ -XXX,XX +XXX,XX @@ typedef struct { QemuOpts *opts; } VMDKCreateOptsData; -static BlockBackend * coroutine_fn GRAPH_RDLOCK +static BlockBackend * coroutine_fn GRAPH_UNLOCKED vmdk_co_create_opts_cb(int64_t size, int idx, bool flat, bool split, bool compress, bool zeroed_grain, void *opaque, Error **errp) @@ -XXX,XX +XXX,XX @@ exit: return blk; } -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED vmdk_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp) { @@ -XXX,XX +XXX,XX @@ exit: return ret; } -static BlockBackend * coroutine_fn vmdk_co_create_cb(int64_t size, int idx, - bool flat, bool split, - bool compress, - bool zeroed_grain, - void *opaque, Error **errp) +static BlockBackend * coroutine_fn GRAPH_UNLOCKED +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; @@ -XXX,XX +XXX,XX @@ static BlockBackend * coroutine_fn vmdk_co_create_cb(int64_t size, int idx, return blk; } -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED vmdk_co_create(BlockdevCreateOptions *create_options, Error **errp) { BlockdevCreateOptionsVmdk *opts; diff --git a/block/vpc.c b/block/vpc.c index XXXXXXX..XXXXXXX 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -XXX,XX +XXX,XX @@ static int calculate_rounded_image_size(BlockdevCreateOptionsVpc *vpc_opts, return 0; } -static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts, - Error **errp) +static int coroutine_fn GRAPH_UNLOCKED +vpc_co_create(BlockdevCreateOptions *opts, Error **errp) { BlockdevCreateOptionsVpc *vpc_opts; BlockBackend *blk = NULL; @@ -XXX,XX +XXX,XX @@ out: return ret; } -static int coroutine_fn GRAPH_RDLOCK +static int coroutine_fn GRAPH_UNLOCKED vpc_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp) { -- 2.40.1 There are some error paths in blk_exp_add() that jump to 'fail:' before 'exp' is even created. So we can't just unconditionally access exp->blk. Add a NULL check, and switch from exp->blk to blk, which is available earlier, just to be extra sure that we really cover all cases where BlockDevOps could have been set for it (in practice, this only happens in drv->create() today, so this part of the change isn't strictly necessary). Fixes: Coverity CID 1509238 Fixes: de79b52604e43fdeba6cee4f5af600b62169f2d2 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-3-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Tested-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/export/export.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/block/export/export.c b/block/export/export.c index XXXXXXX..XXXXXXX 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -XXX,XX +XXX,XX @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) return exp; fail: - blk_set_dev_ops(exp->blk, NULL, NULL); - blk_unref(blk); + if (blk) { + blk_set_dev_ops(blk, NULL, NULL); + blk_unref(blk); + } aio_context_release(ctx); if (exp) { g_free(exp->id); -- 2.40.1 qcow2_do_open() calls a few no_co_wrappers that wrap functions taking the graph lock internally as a writer. Therefore, it can't hold the reader lock across these calls, it causes deadlocks. Drop the lock temporarily around the calls. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-4-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ qcow2_do_open(BlockDriverState *bs, QDict *options, int flags, if (open_data_file) { /* Open external data file */ + bdrv_graph_co_rdunlock(); s->data_file = bdrv_co_open_child(NULL, options, "data-file", bs, &child_of_bds, BDRV_CHILD_DATA, true, errp); + bdrv_graph_co_rdlock(); if (*errp) { ret = -EINVAL; goto fail; @@ -XXX,XX +XXX,XX @@ qcow2_do_open(BlockDriverState *bs, QDict *options, int flags, if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) { if (!s->data_file && s->image_data_file) { + bdrv_graph_co_rdunlock(); s->data_file = bdrv_co_open_child(s->image_data_file, options, "data-file", bs, &child_of_bds, BDRV_CHILD_DATA, false, errp); + bdrv_graph_co_rdlock(); if (!s->data_file) { ret = -EINVAL; goto fail; @@ -XXX,XX +XXX,XX @@ qcow2_do_open(BlockDriverState *bs, QDict *options, int flags, fail: g_free(s->image_data_file); if (open_data_file && has_data_file(bs)) { + bdrv_graph_co_rdunlock(); bdrv_unref_child(bs, s->data_file); + bdrv_graph_co_rdlock(); s->data_file = NULL; } g_free(s->unknown_header_fields); -- 2.40.1 If we take a reader lock, we can't call any functions that take a writer lock internally without causing deadlocks once the reader lock is actually enforced in the main thread, too. Take the reader lock only where it is actually needed. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-5-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qemu-img.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index XXXXXXX..XXXXXXX 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -XXX,XX +XXX,XX @@ static BlockGraphInfoList *collect_image_info_list(bool image_opts, } bs = blk_bs(blk); - GRAPH_RDLOCK_GUARD_MAINLOOP(); - /* * Note that the returned BlockGraphInfo object will not have * information about this image's backing node, because we have opened @@ -XXX,XX +XXX,XX @@ static BlockGraphInfoList *collect_image_info_list(bool image_opts, * duplicate the backing chain information that we obtain by walking * the chain manually here. */ + bdrv_graph_rdlock_main_loop(); bdrv_query_block_graph_info(bs, &info, &err); + bdrv_graph_rdunlock_main_loop(); + if (err) { error_report_err(err); blk_unref(blk); -- 2.40.1 If we take a reader lock, we can't call any functions that take a writer lock internally without causing deadlocks once the reader lock is actually enforced in the main thread, too. Take the reader lock only where it is actually needed. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-6-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/unit/test-bdrv-drain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void coroutine_fn test_co_delete_by_drain(void *opaque) void *buffer = g_malloc(65536); QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buffer, 65536); - GRAPH_RDLOCK_GUARD(); - /* Pretend some internal write operation from parent to child. * Important: We have to read from the child, not from the parent! * Draining works by first propagating it all up the tree to the @@ -XXX,XX +XXX,XX @@ static void coroutine_fn test_co_delete_by_drain(void *opaque) * everything will be drained before we go back down the tree, but * we do not want that. We want to be in the middle of draining * when this following requests returns. */ + bdrv_graph_co_rdlock(); bdrv_co_preadv(tts->wait_child, 0, 65536, &qiov, 0); + bdrv_graph_co_rdunlock(); g_assert_cmpint(bs->refcnt, ==, 1); -- 2.40.1 bdrv_unref() is a no_coroutine_fn, so calling it from coroutine context is invalid. Use bdrv_co_unref() instead. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-7-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/unit/test-bdrv-drain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void coroutine_fn test_co_delete_by_drain(void *opaque) g_assert_cmpint(bs->refcnt, ==, 1); if (!dbdd->detach_instead_of_delete) { - blk_unref(blk); + blk_co_unref(blk); } else { BdrvChild *c, *next_c; QLIST_FOREACH_SAFE(c, &bs->children, next, next_c) { -- 2.40.1 When jobs are sleeping, for example to enforce a given rate limit, they can be reentered early, in particular in order to get paused, to update the rate limit or to get cancelled. Before this patch, they behave in this case as if they had fully completed their rate limiting delay. This means that requests are sped up beyond their limit, violating the constraints that the user gave us. Change the block jobs to sleep in a loop until the necessary delay is completed, while still allowing cancelling them immediately as well pausing (handled by the pause point in job_sleep_ns()) and updating the rate limit. This change is also motivated by iotests cases being prone to fail because drain operations pause and unpause them so often that block jobs complete earlier than they are supposed to. In particular, the next commit would fail iotests 030 without this change. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-8-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/blockjob_int.h | 14 ++++++++++---- block/commit.c | 7 ++----- block/mirror.c | 23 ++++++++++------------- block/stream.c | 7 ++----- blockjob.c | 22 ++++++++++++++++++++-- 5 files changed, 44 insertions(+), 29 deletions(-) diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/blockjob_int.h +++ b/include/block/blockjob_int.h @@ -XXX,XX +XXX,XX @@ void block_job_user_resume(Job *job); */ /** - * block_job_ratelimit_get_delay: + * block_job_ratelimit_processed_bytes: * - * Calculate and return delay for the next request in ns. See the documentation - * of ratelimit_calculate_delay() for details. + * To be called after some work has been done. Adjusts the delay for the next + * request. See the documentation of ratelimit_calculate_delay() for details. */ -int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n); +void block_job_ratelimit_processed_bytes(BlockJob *job, uint64_t n); + +/** + * Put the job to sleep (assuming that it wasn't canceled) to throttle it to the + * right speed according to its rate limiting. + */ +void block_job_ratelimit_sleep(BlockJob *job); /** * block_job_error_action: diff --git a/block/commit.c b/block/commit.c index XXXXXXX..XXXXXXX 100644 --- a/block/commit.c +++ b/block/commit.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn commit_run(Job *job, Error **errp) { CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); int64_t offset; - uint64_t delay_ns = 0; int ret = 0; int64_t n = 0; /* bytes */ QEMU_AUTO_VFREE void *buf = NULL; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn commit_run(Job *job, Error **errp) /* Note that even when no rate limit is applied we need to yield * with no pending I/O here so that bdrv_drain_all() returns. */ - job_sleep_ns(&s->common.job, delay_ns); + block_job_ratelimit_sleep(&s->common); if (job_is_cancelled(&s->common.job)) { break; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn commit_run(Job *job, Error **errp) job_progress_update(&s->common.job, n); if (copy) { - delay_ns = block_job_ratelimit_get_delay(&s->common, n); - } else { - delay_ns = 0; + block_job_ratelimit_processed_bytes(&s->common, n); } } diff --git a/block/mirror.c b/block/mirror.c index XXXXXXX..XXXXXXX 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -XXX,XX +XXX,XX @@ static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset, return bytes_handled; } -static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) +static void coroutine_fn mirror_iteration(MirrorBlockJob *s) { BlockDriverState *source = s->mirror_top_bs->backing->bs; MirrorOp *pseudo_op; int64_t offset; - uint64_t delay_ns = 0, ret = 0; /* At least the first dirty chunk is mirrored in one iteration. */ int nb_chunks = 1; bool write_zeroes_ok = bdrv_can_write_zeroes_with_unmap(blk_bs(s->target)); @@ -XXX,XX +XXX,XX @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) assert(io_bytes); offset += io_bytes; nb_chunks -= DIV_ROUND_UP(io_bytes, s->granularity); - delay_ns = block_job_ratelimit_get_delay(&s->common, io_bytes_acct); + block_job_ratelimit_processed_bytes(&s->common, io_bytes_acct); } - ret = delay_ns; fail: QTAILQ_REMOVE(&s->ops_in_flight, pseudo_op, next); qemu_co_queue_restart_all(&pseudo_op->waiting_requests); g_free(pseudo_op); - - return ret; } static void mirror_free_init(MirrorBlockJob *s) @@ -XXX,XX +XXX,XX @@ static int coroutine_fn mirror_run(Job *job, Error **errp) assert(!s->dbi); s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap); for (;;) { - uint64_t delay_ns = 0; int64_t cnt, delta; bool should_complete; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn mirror_run(Job *job, Error **errp) mirror_wait_for_free_in_flight_slot(s); continue; } else if (cnt != 0) { - delay_ns = mirror_iteration(s); + mirror_iteration(s); } } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn mirror_run(Job *job, Error **errp) } if (job_is_ready(&s->common.job) && !should_complete) { - delay_ns = (s->in_flight == 0 && - cnt == 0 ? BLOCK_JOB_SLICE_TIME : 0); + if (s->in_flight == 0 && cnt == 0) { + trace_mirror_before_sleep(s, cnt, job_is_ready(&s->common.job), + BLOCK_JOB_SLICE_TIME); + job_sleep_ns(&s->common.job, BLOCK_JOB_SLICE_TIME); + } + } else { + block_job_ratelimit_sleep(&s->common); } - trace_mirror_before_sleep(s, cnt, job_is_ready(&s->common.job), - delay_ns); - job_sleep_ns(&s->common.job, delay_ns); s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME); } diff --git a/block/stream.c b/block/stream.c index XXXXXXX..XXXXXXX 100644 --- a/block/stream.c +++ b/block/stream.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn stream_run(Job *job, Error **errp) BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs); int64_t len; int64_t offset = 0; - uint64_t delay_ns = 0; int error = 0; int64_t n = 0; /* bytes */ @@ -XXX,XX +XXX,XX @@ static int coroutine_fn stream_run(Job *job, Error **errp) /* Note that even when no rate limit is applied we need to yield * with no pending I/O here so that bdrv_drain_all() returns. */ - job_sleep_ns(&s->common.job, delay_ns); + block_job_ratelimit_sleep(&s->common); if (job_is_cancelled(&s->common.job)) { break; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn stream_run(Job *job, Error **errp) /* Publish progress */ job_progress_update(&s->common.job, n); if (copy) { - delay_ns = block_job_ratelimit_get_delay(&s->common, n); - } else { - delay_ns = 0; + block_job_ratelimit_processed_bytes(&s->common, n); } } diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) return block_job_set_speed_locked(job, speed, errp); } -int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n) +void block_job_ratelimit_processed_bytes(BlockJob *job, uint64_t n) { IO_CODE(); - return ratelimit_calculate_delay(&job->limit, n); + ratelimit_calculate_delay(&job->limit, n); +} + +void block_job_ratelimit_sleep(BlockJob *job) +{ + uint64_t delay_ns; + + /* + * Sleep at least once. If the job is reentered early, keep waiting until + * we've waited for the full time that is necessary to keep the job at the + * right speed. + * + * Make sure to recalculate the delay after each (possibly interrupted) + * sleep because the speed can change while the job has yielded. + */ + do { + delay_ns = ratelimit_calculate_delay(&job->limit, 0); + job_sleep_ns(&job->job, delay_ns); + } while (delay_ns && !job_is_cancelled(&job->job)); } BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp) -- 2.40.1 There are some conditions under which we don't actually need to do anything for taking a reader lock: Writing the graph is only possible from the main context while holding the BQL. So if a reader is running in the main context under the BQL and knows that it won't be interrupted until the next writer runs, we don't actually need to do anything. This is the case if the reader code neither has a nested event loop (this is forbidden anyway while you hold the lock) nor is a coroutine (because a writer could run when the coroutine has yielded). These conditions are exactly what bdrv_graph_rdlock_main_loop() asserts. They are not fulfilled in bdrv_graph_co_rdlock(), which always runs in a coroutine. This deletes the shortcuts in bdrv_graph_co_rdlock() that skip taking the reader lock in the main thread. Reported-by: Fiona Ebner <f.ebner@proxmox.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-9-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/graph-lock.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/block/graph-lock.c b/block/graph-lock.c index XXXXXXX..XXXXXXX 100644 --- a/block/graph-lock.c +++ b/block/graph-lock.c @@ -XXX,XX +XXX,XX @@ void coroutine_fn bdrv_graph_co_rdlock(void) BdrvGraphRWlock *bdrv_graph; bdrv_graph = qemu_get_current_aio_context()->bdrv_graph; - /* Do not lock if in main thread */ - if (qemu_in_main_thread()) { - return; - } - for (;;) { qatomic_set(&bdrv_graph->reader_count, bdrv_graph->reader_count + 1); @@ -XXX,XX +XXX,XX @@ void coroutine_fn bdrv_graph_co_rdunlock(void) BdrvGraphRWlock *bdrv_graph; bdrv_graph = qemu_get_current_aio_context()->bdrv_graph; - /* Do not lock if in main thread */ - if (qemu_in_main_thread()) { - return; - } - qatomic_store_release(&bdrv_graph->reader_count, bdrv_graph->reader_count - 1); /* make sure writer sees reader_count before we check has_writer */ -- 2.40.1 Skip TestBlockdevReopen.test_insert_compress_filter() if the 'compress' driver isn't available. In order to make the test succeed when the case is skipped, we also need to remove any output from it (which would be missing in the case where we skip it). This is done by replacing qemu_io_log() with qemu_io(). In case of failure, qemu_io() raises an exception with the output of the qemu-io binary in its message, so we don't actually lose anything. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230511143801.255021-1-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/245 | 7 ++++--- tests/qemu-iotests/245.out | 9 +-------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/245 +++ b/tests/qemu-iotests/245 @@ -XXX,XX +XXX,XX @@ class TestBlockdevReopen(iotests.QMPTestCase): self.reopen(hd0_opts, {'file': 'hd0-file'}) # Insert (and remove) a compress filter + @iotests.skip_if_unsupported(['compress']) def test_insert_compress_filter(self): # Add an image to the VM: hd (raw) -> hd0 (qcow2) -> hd0-file (file) opts = {'driver': 'raw', 'node-name': 'hd', 'file': hd_opts(0)} @@ -XXX,XX +XXX,XX @@ class TestBlockdevReopen(iotests.QMPTestCase): # Check the first byte of the first three L2 entries and verify that # the second one is compressed (0x40) while the others are not (0x80) - iotests.qemu_io_log('-f', 'raw', '-c', 'read -P 0x80 0x40000 1', - '-c', 'read -P 0x40 0x40008 1', - '-c', 'read -P 0x80 0x40010 1', hd_path[0]) + iotests.qemu_io('-f', 'raw', '-c', 'read -P 0x80 0x40000 1', + '-c', 'read -P 0x40 0x40008 1', + '-c', 'read -P 0x80 0x40010 1', hd_path[0]) # Swap the disk images of two active block devices def test_swap_files(self): diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/245.out +++ b/tests/qemu-iotests/245.out @@ -XXX,XX +XXX,XX @@ {"return": {}} {"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} {"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} -....read 1/1 bytes at offset 262144 -1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 1/1 bytes at offset 262152 -1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 1/1 bytes at offset 262160 -1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) - -................ +.................... ---------------------------------------------------------------------- Ran 26 tests -- 2.40.1 From: Stefan Hajnoczi <stefanha@redhat.com> QEMU's event loop supports nesting, which means that event handler functions may themselves call aio_poll(). The condition that triggered a handler must be reset before the nested aio_poll() call, otherwise the same handler will be called and immediately re-enter aio_poll. This leads to an infinite loop and stack exhaustion. Poll handlers are especially prone to this issue, because they typically reset their condition by finishing the processing of pending work. Unfortunately it is during the processing of pending work that nested aio_poll() calls typically occur and the condition has not yet been reset. Disable a poll handler during ->io_poll_ready() so that a nested aio_poll() call cannot invoke ->io_poll_ready() again. As a result, the disabled poll handler and its associated fd handler do not run during the nested aio_poll(). Calling aio_set_fd_handler() from inside nested aio_poll() could cause it to run again. If the fd handler is pending inside nested aio_poll(), then it will also run again. In theory fd handlers can be affected by the same issue, but they are more likely to reset the condition before calling nested aio_poll(). This is a special case and it's somewhat complex, but I don't see a way around it as long as nested aio_poll() is supported. Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2186181 Fixes: c38270692593 ("block: Mark bdrv_co_io_(un)plug() and callers GRAPH_RDLOCK") Cc: Kevin Wolf <kwolf@redhat.com> Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20230502184134.534703-2-stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- util/aio-posix.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/util/aio-posix.c b/util/aio-posix.c index XXXXXXX..XXXXXXX 100644 --- a/util/aio-posix.c +++ b/util/aio-posix.c @@ -XXX,XX +XXX,XX @@ static bool aio_dispatch_handler(AioContext *ctx, AioHandler *node) poll_ready && revents == 0 && aio_node_check(ctx, node->is_external) && node->io_poll_ready) { + /* + * Remove temporarily to avoid infinite loops when ->io_poll_ready() + * calls aio_poll() before clearing the condition that made the poll + * handler become ready. + */ + QLIST_SAFE_REMOVE(node, node_poll); + node->io_poll_ready(node->opaque); + if (!QLIST_IS_INSERTED(node, node_poll)) { + QLIST_INSERT_HEAD(&ctx->poll_aio_handlers, node, node_poll); + } + /* * Return early since revents was zero. aio_notify() does not count as * progress. -- 2.40.1 From: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20230502184134.534703-3-stefanha@redhat.com> Tested-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/unit/test-nested-aio-poll.c | 130 ++++++++++++++++++++++++++++++ tests/unit/meson.build | 1 + 2 files changed, 131 insertions(+) create mode 100644 tests/unit/test-nested-aio-poll.c diff --git a/tests/unit/test-nested-aio-poll.c b/tests/unit/test-nested-aio-poll.c new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/unit/test-nested-aio-poll.c @@ -XXX,XX +XXX,XX @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Test that poll handlers are not re-entrant in nested aio_poll() + * + * Copyright Red Hat + * + * Poll handlers are usually level-triggered. That means they continue firing + * until the condition is reset (e.g. a virtqueue becomes empty). If a poll + * handler calls nested aio_poll() before the condition is reset, then infinite + * recursion occurs. + * + * aio_poll() is supposed to prevent this by disabling poll handlers in nested + * aio_poll() calls. This test case checks that this is indeed what happens. + */ +#include "qemu/osdep.h" +#include "block/aio.h" +#include "qapi/error.h" + +typedef struct { + AioContext *ctx; + + /* This is the EventNotifier that drives the test */ + EventNotifier poll_notifier; + + /* This EventNotifier is only used to wake aio_poll() */ + EventNotifier dummy_notifier; + + bool nested; +} TestData; + +static void io_read(EventNotifier *notifier) +{ + fprintf(stderr, "%s %p\n", __func__, notifier); + event_notifier_test_and_clear(notifier); +} + +static bool io_poll_true(void *opaque) +{ + fprintf(stderr, "%s %p\n", __func__, opaque); + return true; +} + +static bool io_poll_false(void *opaque) +{ + fprintf(stderr, "%s %p\n", __func__, opaque); + return false; +} + +static void io_poll_ready(EventNotifier *notifier) +{ + TestData *td = container_of(notifier, TestData, poll_notifier); + + fprintf(stderr, "> %s\n", __func__); + + g_assert(!td->nested); + td->nested = true; + + /* Wake the following nested aio_poll() call */ + event_notifier_set(&td->dummy_notifier); + + /* This nested event loop must not call io_poll()/io_poll_ready() */ + g_assert(aio_poll(td->ctx, true)); + + td->nested = false; + + fprintf(stderr, "< %s\n", __func__); +} + +/* dummy_notifier never triggers */ +static void io_poll_never_ready(EventNotifier *notifier) +{ + g_assert_not_reached(); +} + +static void test(void) +{ + TestData td = { + .ctx = aio_context_new(&error_abort), + }; + + qemu_set_current_aio_context(td.ctx); + + /* Enable polling */ + aio_context_set_poll_params(td.ctx, 1000000, 2, 2, &error_abort); + + /* + * The GSource is unused but this has the side-effect of changing the fdmon + * that AioContext uses. + */ + aio_get_g_source(td.ctx); + + /* Make the event notifier active (set) right away */ + event_notifier_init(&td.poll_notifier, 1); + aio_set_event_notifier(td.ctx, &td.poll_notifier, false, + io_read, io_poll_true, io_poll_ready); + + /* This event notifier will be used later */ + event_notifier_init(&td.dummy_notifier, 0); + aio_set_event_notifier(td.ctx, &td.dummy_notifier, false, + io_read, io_poll_false, io_poll_never_ready); + + /* Consume aio_notify() */ + g_assert(!aio_poll(td.ctx, false)); + + /* + * Run the io_read() handler. This has the side-effect of activating + * polling in future aio_poll() calls. + */ + g_assert(aio_poll(td.ctx, true)); + + /* The second time around the io_poll()/io_poll_ready() handler runs */ + g_assert(aio_poll(td.ctx, true)); + + /* Run io_poll()/io_poll_ready() one more time to show it keeps working */ + g_assert(aio_poll(td.ctx, true)); + + aio_set_event_notifier(td.ctx, &td.dummy_notifier, false, + NULL, NULL, NULL); + aio_set_event_notifier(td.ctx, &td.poll_notifier, false, NULL, NULL, NULL); + event_notifier_cleanup(&td.dummy_notifier); + event_notifier_cleanup(&td.poll_notifier); + aio_context_unref(td.ctx); +} + +int main(int argc, char **argv) +{ + g_test_init(&argc, &argv, NULL); + g_test_add_func("/nested-aio-poll", test); + return g_test_run(); +} diff --git a/tests/unit/meson.build b/tests/unit/meson.build index XXXXXXX..XXXXXXX 100644 --- a/tests/unit/meson.build +++ b/tests/unit/meson.build @@ -XXX,XX +XXX,XX @@ if have_block 'test-coroutine': [testblock], 'test-aio': [testblock], 'test-aio-multithread': [testblock], + 'test-nested-aio-poll': [testblock], 'test-throttle': [testblock], 'test-thread-pool': [testblock], 'test-hbitmap': [testblock], -- 2.40.1