From 66c8672d242e9a494216237648b7787649c2c970 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 24 Mar 2020 18:36:26 +0300 Subject: [PATCH 01/10] block/mirror: fix use after free of local_err local_err is used again in mirror_exit_common() after bdrv_set_backing_hd(), so we must zero it. Otherwise try to set non-NULL local_err will crash. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20200324153630.11882-3-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake Reviewed-by: John Snow Signed-off-by: Max Reitz --- block/mirror.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/mirror.c b/block/mirror.c index 447051dbc6..6203e5946e 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job) bdrv_set_backing_hd(target_bs, backing, &local_err); if (local_err) { error_report_err(local_err); + local_err = NULL; ret = -EPERM; } } From b92902dfeaafbceaf744ab7473f2d070284f6172 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Thu, 26 Mar 2020 03:12:17 +0200 Subject: [PATCH 02/10] block: pass BlockDriver reference to the .bdrv_co_create This will allow the reuse of a single generic .bdrv_co_create implementation for several drivers. No functional changes. Signed-off-by: Maxim Levitsky Message-Id: <20200326011218.29230-2-mlevitsk@redhat.com> Reviewed-by: Denis V. Lunev Signed-off-by: Max Reitz --- block.c | 3 ++- block/crypto.c | 3 ++- block/file-posix.c | 4 +++- block/file-win32.c | 4 +++- block/gluster.c | 3 ++- block/nfs.c | 4 +++- block/parallels.c | 3 ++- block/qcow.c | 3 ++- block/qcow2.c | 4 +++- block/qed.c | 3 ++- block/raw-format.c | 4 +++- block/rbd.c | 3 ++- block/sheepdog.c | 4 +++- block/ssh.c | 4 +++- block/vdi.c | 4 +++- block/vhdx.c | 3 ++- block/vmdk.c | 4 +++- block/vpc.c | 6 ++++-- include/block/block_int.h | 3 ++- 19 files changed, 49 insertions(+), 20 deletions(-) diff --git a/block.c b/block.c index cccae5add9..ff23e20443 100644 --- a/block.c +++ b/block.c @@ -483,7 +483,8 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque) CreateCo *cco = opaque; assert(cco->drv); - ret = cco->drv->bdrv_co_create_opts(cco->filename, cco->opts, &local_err); + ret = cco->drv->bdrv_co_create_opts(cco->drv, + cco->filename, cco->opts, &local_err); error_propagate(&cco->err, local_err); cco->ret = ret; } diff --git a/block/crypto.c b/block/crypto.c index 4425ebeb47..d577f89659 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -601,7 +601,8 @@ fail: return ret; } -static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, +static int coroutine_fn block_crypto_co_create_opts_luks(BlockDriver *drv, + const char *filename, QemuOpts *opts, Error **errp) { diff --git a/block/file-posix.c b/block/file-posix.c index 9bc3838b2a..65bc980bc4 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -2405,7 +2405,9 @@ out: return result; } -static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, +static int coroutine_fn raw_co_create_opts(BlockDriver *drv, + const char *filename, + QemuOpts *opts, Error **errp) { BlockdevCreateOptions options; diff --git a/block/file-win32.c b/block/file-win32.c index 77e8ff7b68..15859839a1 100644 --- a/block/file-win32.c +++ b/block/file-win32.c @@ -588,7 +588,9 @@ static int raw_co_create(BlockdevCreateOptions *options, Error **errp) return 0; } -static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, +static int coroutine_fn raw_co_create_opts(BlockDriver *drv, + const char *filename, + QemuOpts *opts, Error **errp) { BlockdevCreateOptions options; diff --git a/block/gluster.c b/block/gluster.c index 4fa4a77a47..0aa1f2cda4 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -1130,7 +1130,8 @@ out: return ret; } -static int coroutine_fn qemu_gluster_co_create_opts(const char *filename, +static int coroutine_fn qemu_gluster_co_create_opts(BlockDriver *drv, + const char *filename, QemuOpts *opts, Error **errp) { diff --git a/block/nfs.c b/block/nfs.c index 9a6311e270..cc2413d5ab 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -662,7 +662,9 @@ out: return ret; } -static int coroutine_fn nfs_file_co_create_opts(const char *url, QemuOpts *opts, +static int coroutine_fn nfs_file_co_create_opts(BlockDriver *drv, + const char *url, + QemuOpts *opts, Error **errp) { BlockdevCreateOptions *create_options; diff --git a/block/parallels.c b/block/parallels.c index 7a01997659..6d4ed77f16 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -609,7 +609,8 @@ exit: goto out; } -static int coroutine_fn parallels_co_create_opts(const char *filename, +static int coroutine_fn parallels_co_create_opts(BlockDriver *drv, + const char *filename, QemuOpts *opts, Error **errp) { diff --git a/block/qcow.c b/block/qcow.c index fce8989868..8973e4e565 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -934,7 +934,8 @@ exit: return ret; } -static int coroutine_fn qcow_co_create_opts(const char *filename, +static int coroutine_fn qcow_co_create_opts(BlockDriver *drv, + const char *filename, QemuOpts *opts, Error **errp) { BlockdevCreateOptions *create_options = NULL; diff --git a/block/qcow2.c b/block/qcow2.c index d1da3d91db..5f65fce924 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3558,7 +3558,9 @@ out: return ret; } -static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opts, +static int coroutine_fn qcow2_co_create_opts(BlockDriver *drv, + const char *filename, + QemuOpts *opts, Error **errp) { BlockdevCreateOptions *create_options = NULL; diff --git a/block/qed.c b/block/qed.c index d8c4e5fb1e..1af9b3cb1d 100644 --- a/block/qed.c +++ b/block/qed.c @@ -720,7 +720,8 @@ out: return ret; } -static int coroutine_fn bdrv_qed_co_create_opts(const char *filename, +static int coroutine_fn 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 3a76ec7dd2..93b25e1b6b 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -419,7 +419,9 @@ static int raw_has_zero_init_truncate(BlockDriverState *bs) return bdrv_has_zero_init_truncate(bs->file->bs); } -static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, +static int coroutine_fn raw_co_create_opts(BlockDriver *drv, + const char *filename, + QemuOpts *opts, Error **errp) { return bdrv_create_file(filename, opts, errp); diff --git a/block/rbd.c b/block/rbd.c index 84115d34b4..e637639a07 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -437,7 +437,8 @@ static int qemu_rbd_co_create(BlockdevCreateOptions *options, Error **errp) return qemu_rbd_do_create(options, NULL, NULL, errp); } -static int coroutine_fn qemu_rbd_co_create_opts(const char *filename, +static int coroutine_fn qemu_rbd_co_create_opts(BlockDriver *drv, + const char *filename, QemuOpts *opts, Error **errp) { diff --git a/block/sheepdog.c b/block/sheepdog.c index cfa84338a2..a8a7e32a41 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -2157,7 +2157,9 @@ out: return ret; } -static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts, +static int coroutine_fn sd_co_create_opts(BlockDriver *drv, + const char *filename, + QemuOpts *opts, Error **errp) { BlockdevCreateOptions *create_options = NULL; diff --git a/block/ssh.c b/block/ssh.c index b4375cf7d2..84e92821c0 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -963,7 +963,9 @@ fail: return ret; } -static int coroutine_fn ssh_co_create_opts(const char *filename, QemuOpts *opts, +static int coroutine_fn ssh_co_create_opts(BlockDriver *drv, + const char *filename, + QemuOpts *opts, Error **errp) { BlockdevCreateOptions *create_options; diff --git a/block/vdi.c b/block/vdi.c index 0142da7233..e1a11f2aa0 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -896,7 +896,9 @@ static int coroutine_fn vdi_co_create(BlockdevCreateOptions *create_options, return vdi_co_do_create(create_options, DEFAULT_CLUSTER_SIZE, errp); } -static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, +static int coroutine_fn vdi_co_create_opts(BlockDriver *drv, + const char *filename, + QemuOpts *opts, Error **errp) { QDict *qdict = NULL; diff --git a/block/vhdx.c b/block/vhdx.c index f02d2611be..33e57cd656 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -2046,7 +2046,8 @@ delete_and_exit: return ret; } -static int coroutine_fn vhdx_co_create_opts(const char *filename, +static int coroutine_fn vhdx_co_create_opts(BlockDriver *drv, + const char *filename, QemuOpts *opts, Error **errp) { diff --git a/block/vmdk.c b/block/vmdk.c index 8466051bc9..218d9c9800 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -2588,7 +2588,9 @@ exit: return blk; } -static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts, +static int coroutine_fn vmdk_co_create_opts(BlockDriver *drv, + const char *filename, + QemuOpts *opts, Error **errp) { Error *local_err = NULL; diff --git a/block/vpc.c b/block/vpc.c index a65550298e..6df75e22dc 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -1089,8 +1089,10 @@ out: return ret; } -static int coroutine_fn vpc_co_create_opts(const char *filename, - QemuOpts *opts, Error **errp) +static int coroutine_fn vpc_co_create_opts(BlockDriver *drv, + const char *filename, + QemuOpts *opts, + Error **errp) { BlockdevCreateOptions *create_options = NULL; QDict *qdict; diff --git a/include/block/block_int.h b/include/block/block_int.h index ae9c4da4d0..57c8ea24b2 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -135,7 +135,8 @@ struct BlockDriver { void (*bdrv_close)(BlockDriverState *bs); int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts, Error **errp); - int coroutine_fn (*bdrv_co_create_opts)(const char *filename, + int coroutine_fn (*bdrv_co_create_opts)(BlockDriver *drv, + const char *filename, QemuOpts *opts, Error **errp); int (*bdrv_make_empty)(BlockDriverState *bs); From 5a5e7f8cd86b7ced0732b1b6e28c82baa65b09c9 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Thu, 26 Mar 2020 03:12:18 +0200 Subject: [PATCH 03/10] block: trickle down the fallback image creation function use to the block drivers Instead of checking the .bdrv_co_create_opts to see if we need the fallback, just implement the .bdrv_co_create_opts in the drivers that need it. This way we don't break various places that need to know if the underlying protocol/format really supports image creation, and this way we still allow some drivers to not support image creation. Fixes: fd17146cd93d1704cd96d7c2757b325fc7aac6fd Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1816007 Note that technically this driver reverts the image creation fallback for the vxhs driver since I don't have a means to test it, and IMHO it is better to leave it not supported as it was prior to generic image creation patches. Also drop iscsi_create_opts which was left accidentally. Signed-off-by: Maxim Levitsky Message-Id: <20200326011218.29230-3-mlevitsk@redhat.com> Reviewed-by: Denis V. Lunev [mreitz: Fixed alignment, and moved bdrv_co_create_opts_simple() and bdrv_create_opts_simple from block.h into block_int.h] Signed-off-by: Max Reitz --- block.c | 35 ++++++++++++++++++++--------------- block/file-posix.c | 7 ++++++- block/iscsi.c | 16 ++++------------ block/nbd.c | 6 ++++++ block/nvme.c | 3 +++ include/block/block.h | 1 + include/block/block_int.h | 11 +++++++++++ 7 files changed, 51 insertions(+), 28 deletions(-) diff --git a/block.c b/block.c index ff23e20443..af3faf664e 100644 --- a/block.c +++ b/block.c @@ -598,8 +598,15 @@ static int create_file_fallback_zero_first_sector(BlockBackend *blk, return 0; } -static int bdrv_create_file_fallback(const char *filename, BlockDriver *drv, - QemuOpts *opts, Error **errp) +/** + * Simple implementation of bdrv_co_create_opts for protocol drivers + * which only support creation via opening a file + * (usually existing raw storage device) + */ +int coroutine_fn bdrv_co_create_opts_simple(BlockDriver *drv, + const char *filename, + QemuOpts *opts, + Error **errp) { BlockBackend *blk; QDict *options; @@ -663,11 +670,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp) return -ENOENT; } - if (drv->bdrv_co_create_opts) { - return bdrv_create(drv, filename, opts, errp); - } else { - return bdrv_create_file_fallback(filename, drv, opts, errp); - } + return bdrv_create(drv, filename, opts, errp); } int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp) @@ -1592,9 +1595,9 @@ QemuOptsList bdrv_runtime_opts = { }, }; -static QemuOptsList fallback_create_opts = { - .name = "fallback-create-opts", - .head = QTAILQ_HEAD_INITIALIZER(fallback_create_opts.head), +QemuOptsList bdrv_create_opts_simple = { + .name = "simple-create-opts", + .head = QTAILQ_HEAD_INITIALIZER(bdrv_create_opts_simple.head), .desc = { { .name = BLOCK_OPT_SIZE, @@ -5963,13 +5966,15 @@ void bdrv_img_create(const char *filename, const char *fmt, return; } + if (!proto_drv->create_opts) { + error_setg(errp, "Protocol driver '%s' does not support image creation", + proto_drv->format_name); + return; + } + /* Create parameter list */ create_opts = qemu_opts_append(create_opts, drv->create_opts); - if (proto_drv->create_opts) { - create_opts = qemu_opts_append(create_opts, proto_drv->create_opts); - } else { - create_opts = qemu_opts_append(create_opts, &fallback_create_opts); - } + create_opts = qemu_opts_append(create_opts, proto_drv->create_opts); opts = qemu_opts_create(create_opts, NULL, 0, &error_abort); diff --git a/block/file-posix.c b/block/file-posix.c index 65bc980bc4..7e19bbff5f 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -3513,6 +3513,8 @@ static BlockDriver bdrv_host_device = { .bdrv_reopen_prepare = raw_reopen_prepare, .bdrv_reopen_commit = raw_reopen_commit, .bdrv_reopen_abort = raw_reopen_abort, + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, .mutable_opts = mutable_opts, .bdrv_co_invalidate_cache = raw_co_invalidate_cache, .bdrv_co_pwrite_zeroes = hdev_co_pwrite_zeroes, @@ -3639,10 +3641,11 @@ static BlockDriver bdrv_host_cdrom = { .bdrv_reopen_prepare = raw_reopen_prepare, .bdrv_reopen_commit = raw_reopen_commit, .bdrv_reopen_abort = raw_reopen_abort, + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, .mutable_opts = mutable_opts, .bdrv_co_invalidate_cache = raw_co_invalidate_cache, - .bdrv_co_preadv = raw_co_preadv, .bdrv_co_pwritev = raw_co_pwritev, .bdrv_co_flush_to_disk = raw_co_flush_to_disk, @@ -3771,6 +3774,8 @@ static BlockDriver bdrv_host_cdrom = { .bdrv_reopen_prepare = raw_reopen_prepare, .bdrv_reopen_commit = raw_reopen_commit, .bdrv_reopen_abort = raw_reopen_abort, + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, .mutable_opts = mutable_opts, .bdrv_co_preadv = raw_co_preadv, diff --git a/block/iscsi.c b/block/iscsi.c index 682abd8e09..14680a436a 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -2399,18 +2399,6 @@ out_unlock: return r; } -static QemuOptsList iscsi_create_opts = { - .name = "iscsi-create-opts", - .head = QTAILQ_HEAD_INITIALIZER(iscsi_create_opts.head), - .desc = { - { - .name = BLOCK_OPT_SIZE, - .type = QEMU_OPT_SIZE, - .help = "Virtual disk size" - }, - { /* end of list */ } - } -}; static const char *const iscsi_strong_runtime_opts[] = { "transport", @@ -2434,6 +2422,8 @@ static BlockDriver bdrv_iscsi = { .bdrv_parse_filename = iscsi_parse_filename, .bdrv_file_open = iscsi_open, .bdrv_close = iscsi_close, + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, .bdrv_reopen_prepare = iscsi_reopen_prepare, .bdrv_reopen_commit = iscsi_reopen_commit, .bdrv_co_invalidate_cache = iscsi_co_invalidate_cache, @@ -2471,6 +2461,8 @@ static BlockDriver bdrv_iser = { .bdrv_parse_filename = iscsi_parse_filename, .bdrv_file_open = iscsi_open, .bdrv_close = iscsi_close, + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, .bdrv_reopen_prepare = iscsi_reopen_prepare, .bdrv_reopen_commit = iscsi_reopen_commit, .bdrv_co_invalidate_cache = iscsi_co_invalidate_cache, diff --git a/block/nbd.c b/block/nbd.c index 976be76647..2160859f64 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -2038,6 +2038,8 @@ static BlockDriver bdrv_nbd = { .protocol_name = "nbd", .instance_size = sizeof(BDRVNBDState), .bdrv_parse_filename = nbd_parse_filename, + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, .bdrv_file_open = nbd_open, .bdrv_reopen_prepare = nbd_client_reopen_prepare, .bdrv_co_preadv = nbd_client_co_preadv, @@ -2063,6 +2065,8 @@ static BlockDriver bdrv_nbd_tcp = { .protocol_name = "nbd+tcp", .instance_size = sizeof(BDRVNBDState), .bdrv_parse_filename = nbd_parse_filename, + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, .bdrv_file_open = nbd_open, .bdrv_reopen_prepare = nbd_client_reopen_prepare, .bdrv_co_preadv = nbd_client_co_preadv, @@ -2088,6 +2092,8 @@ static BlockDriver bdrv_nbd_unix = { .protocol_name = "nbd+unix", .instance_size = sizeof(BDRVNBDState), .bdrv_parse_filename = nbd_parse_filename, + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, .bdrv_file_open = nbd_open, .bdrv_reopen_prepare = nbd_client_reopen_prepare, .bdrv_co_preadv = nbd_client_co_preadv, diff --git a/block/nvme.c b/block/nvme.c index d41c4bda6e..7b7c0cc5d6 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -1333,6 +1333,9 @@ static BlockDriver bdrv_nvme = { .protocol_name = "nvme", .instance_size = sizeof(BDRVNVMeState), + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, + .bdrv_parse_filename = nvme_parse_filename, .bdrv_file_open = nvme_file_open, .bdrv_close = nvme_close, diff --git a/include/block/block.h b/include/block/block.h index e569a4d747..b05995fe9c 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -283,6 +283,7 @@ BlockDriver *bdrv_find_format(const char *format_name); int bdrv_create(BlockDriver *drv, const char* filename, QemuOpts *opts, Error **errp); int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp); + BlockDriverState *bdrv_new(void); void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, Error **errp); diff --git a/include/block/block_int.h b/include/block/block_int.h index 57c8ea24b2..4c3587ea19 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1331,4 +1331,15 @@ int refresh_total_sectors(BlockDriverState *bs, int64_t hint); void bdrv_set_monitor_owned(BlockDriverState *bs); BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp); +/** + * Simple implementation of bdrv_co_create_opts for protocol drivers + * which only support creation via opening a file + * (usually existing raw storage device) + */ +int coroutine_fn bdrv_co_create_opts_simple(BlockDriver *drv, + const char *filename, + QemuOpts *opts, + Error **errp); +extern QemuOptsList bdrv_create_opts_simple; + #endif /* BLOCK_INT_H */ From a951a631b96cefbe2636fed33a583ca05e9132ba Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 24 Mar 2020 12:42:30 -0500 Subject: [PATCH 04/10] qcow2: Comment typo fixes Various trivial typos noticed while working on this file. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Message-Id: <20200324174233.1622067-2-eblake@redhat.com> Signed-off-by: Max Reitz --- block/qcow2.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 5f65fce924..b565cf912e 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -177,7 +177,7 @@ static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock *block, size_t offset, } -/* +/* * read qcow2 extension and fill bs * start reading from start_offset * finish reading upon magic of value 0 or when end_offset reached @@ -3255,7 +3255,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) * inconsistency later. * * We do need a refcount table because growing the refcount table means - * allocating two new refcount blocks - the seconds of which would be at + * allocating two new refcount blocks - the second of which would be at * 2 GB for 64k clusters, and we don't want to have a 2 GB initial file * size for any qcow2 image. */ @@ -3500,7 +3500,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) goto out; } - /* Want a backing file? There you go.*/ + /* Want a backing file? There you go. */ if (qcow2_opts->has_backing_file) { const char *backing_format = NULL; From bb40ebce2cb0bd4bf37968074d43d5a864fb6dee Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 24 Mar 2020 12:42:31 -0500 Subject: [PATCH 05/10] qcow2: List autoclear bit names in header The feature table is supposed to advertise the name of all feature bits that we support; however, we forgot to update the table for autoclear bits. While at it, move the table to read-only memory in code, and tweak the qcow2 spec to name the second autoclear bit. Update iotests that are affected by the longer header length. Fixes: 88ddffae Fixes: 93c24936 Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20200324174233.1622067-3-eblake@redhat.com> Signed-off-by: Max Reitz --- block/qcow2.c | 12 +++++++++++- docs/interop/qcow2.txt | 3 ++- tests/qemu-iotests/031.out | 8 ++++---- tests/qemu-iotests/036.out | 4 ++-- tests/qemu-iotests/061.out | 14 +++++++------- 5 files changed, 26 insertions(+), 15 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index b565cf912e..b74cbeb047 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2825,7 +2825,7 @@ int qcow2_update_header(BlockDriverState *bs) /* Feature table */ if (s->qcow_version >= 3) { - Qcow2Feature features[] = { + static const Qcow2Feature features[] = { { .type = QCOW2_FEAT_TYPE_INCOMPATIBLE, .bit = QCOW2_INCOMPAT_DIRTY_BITNR, @@ -2846,6 +2846,16 @@ int qcow2_update_header(BlockDriverState *bs) .bit = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR, .name = "lazy refcounts", }, + { + .type = QCOW2_FEAT_TYPE_AUTOCLEAR, + .bit = QCOW2_AUTOCLEAR_BITMAPS_BITNR, + .name = "bitmaps", + }, + { + .type = QCOW2_FEAT_TYPE_AUTOCLEAR, + .bit = QCOW2_AUTOCLEAR_DATA_FILE_RAW_BITNR, + .name = "raw external data", + }, }; ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FEATURE_TABLE, diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt index 5597e24474..640e0eca40 100644 --- a/docs/interop/qcow2.txt +++ b/docs/interop/qcow2.txt @@ -143,7 +143,8 @@ the next fields through header_length. bit is unset, the bitmaps extension data must be considered inconsistent. - Bit 1: If this bit is set, the external data file can + Bit 1: Raw external data bit + If this bit is set, the external data file can be read as a consistent standalone raw image without looking at the qcow2 metadata. diff --git a/tests/qemu-iotests/031.out b/tests/qemu-iotests/031.out index d535e407bc..46f97c5a4e 100644 --- a/tests/qemu-iotests/031.out +++ b/tests/qemu-iotests/031.out @@ -117,7 +117,7 @@ header_length 104 Header extension: magic 0x6803f857 -length 192 +length 288 data Header extension: @@ -150,7 +150,7 @@ header_length 104 Header extension: magic 0x6803f857 -length 192 +length 288 data Header extension: @@ -164,7 +164,7 @@ No errors were found on the image. magic 0x514649fb version 3 -backing_file_offset 0x178 +backing_file_offset 0x1d8 backing_file_size 0x17 cluster_bits 16 size 67108864 @@ -188,7 +188,7 @@ data 'host_device' Header extension: magic 0x6803f857 -length 192 +length 288 data Header extension: diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out index 0b52b934e1..23b699ce06 100644 --- a/tests/qemu-iotests/036.out +++ b/tests/qemu-iotests/036.out @@ -26,7 +26,7 @@ compatible_features [] autoclear_features [63] Header extension: magic 0x6803f857 -length 192 +length 288 data @@ -38,7 +38,7 @@ compatible_features [] autoclear_features [] Header extension: magic 0x6803f857 -length 192 +length 288 data *** done diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out index 8b3091a412..413cc4e0f4 100644 --- a/tests/qemu-iotests/061.out +++ b/tests/qemu-iotests/061.out @@ -26,7 +26,7 @@ header_length 104 Header extension: magic 0x6803f857 -length 192 +length 288 data magic 0x514649fb @@ -84,7 +84,7 @@ header_length 104 Header extension: magic 0x6803f857 -length 192 +length 288 data magic 0x514649fb @@ -140,7 +140,7 @@ header_length 104 Header extension: magic 0x6803f857 -length 192 +length 288 data ERROR cluster 5 refcount=0 reference=1 @@ -195,7 +195,7 @@ header_length 104 Header extension: magic 0x6803f857 -length 192 +length 288 data magic 0x514649fb @@ -264,7 +264,7 @@ header_length 104 Header extension: magic 0x6803f857 -length 192 +length 288 data read 65536/65536 bytes at offset 44040192 @@ -298,7 +298,7 @@ header_length 104 Header extension: magic 0x6803f857 -length 192 +length 288 data ERROR cluster 5 refcount=0 reference=1 @@ -327,7 +327,7 @@ header_length 104 Header extension: magic 0x6803f857 -length 192 +length 288 data read 131072/131072 bytes at offset 0 From e7be13ad3ff593608a118bfb69553037bd221563 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 24 Mar 2020 12:42:32 -0500 Subject: [PATCH 06/10] qcow2: Avoid feature name extension on small cluster size As the feature name table can be quite large (over 9k if all 64 bits of all three feature fields have names; a mere 8 features leaves only 8 bytes for a backing file name in a 512-byte cluster), it is unwise to emit this optional header in images with small cluster sizes. Update iotest 036 to skip running on small cluster sizes; meanwhile, note that iotest 061 never passed on alternative cluster sizes (however, I limited this patch to tests with output affected by adding feature names, rather than auditing for other tests that are not robust to alternative cluster sizes). Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Message-Id: <20200324174233.1622067-4-eblake@redhat.com> Signed-off-by: Max Reitz --- block/qcow2.c | 11 +++++++++-- tests/qemu-iotests/036 | 6 ++++-- tests/qemu-iotests/061 | 6 ++++-- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index b74cbeb047..2bb536b014 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2823,8 +2823,15 @@ int qcow2_update_header(BlockDriverState *bs) buflen -= ret; } - /* Feature table */ - if (s->qcow_version >= 3) { + /* + * Feature table. A mere 8 feature names occupies 392 bytes, and + * when coupled with the v3 minimum header of 104 bytes plus the + * 8-byte end-of-extension marker, that would leave only 8 bytes + * for a backing file name in an image with 512-byte clusters. + * Thus, we choose to omit this header for cluster sizes 4k and + * smaller. + */ + if (s->qcow_version >= 3 && s->cluster_size > 4096) { static const Qcow2Feature features[] = { { .type = QCOW2_FEAT_TYPE_INCOMPATIBLE, diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036 index 512598421c..cf522de7a1 100755 --- a/tests/qemu-iotests/036 +++ b/tests/qemu-iotests/036 @@ -44,8 +44,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt qcow2 _supported_proto file # Only qcow2v3 and later supports feature bits; -# qcow2.py does not support external data files -_unsupported_imgopts 'compat=0.10' data_file +# qcow2.py does not support external data files; +# this test requires a cluster size large enough for the feature table +_unsupported_imgopts 'compat=0.10' data_file \ + 'cluster_size=\(512\|1024\|2048\|4096\)' echo echo === Image with unknown incompatible feature bit === diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061 index 36b040491f..ce285d3084 100755 --- a/tests/qemu-iotests/061 +++ b/tests/qemu-iotests/061 @@ -44,8 +44,10 @@ _supported_os Linux # Conversion between different compat versions can only really work # with refcount_bits=16; # we have explicit tests for data_file here, but the whole test does -# not work with it -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file +# not work with it; +# we have explicit tests for various cluster sizes, the remaining tests +# require the default 64k cluster +_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file cluster_size echo echo "=== Testing version downgrade with zero expansion ===" From ed049910637be991c88cc25c864115bc5b1e4dab Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 24 Mar 2020 12:42:33 -0500 Subject: [PATCH 07/10] sheepdog: Consistently set bdrv_has_zero_init_truncate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit block_int.h claims that .bdrv_has_zero_init must return 0 if .bdrv_has_zero_init_truncate does likewise; but this is violated if only the former callback is provided if .bdrv_co_truncate also exists. When adding the latter callback, it was mistakenly added to only one of the three possible sheepdog instantiations. Fixes: 1dcaf527 Signed-off-by: Eric Blake Message-Id: <20200324174233.1622067-5-eblake@redhat.com> Reviewed-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Max Reitz --- block/sheepdog.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/block/sheepdog.c b/block/sheepdog.c index a8a7e32a41..59f7ebb171 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -3271,6 +3271,7 @@ static BlockDriver bdrv_sheepdog_tcp = { .bdrv_co_create = sd_co_create, .bdrv_co_create_opts = sd_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, + .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1, .bdrv_getlength = sd_getlength, .bdrv_get_allocated_file_size = sd_get_allocated_file_size, .bdrv_co_truncate = sd_co_truncate, @@ -3309,6 +3310,7 @@ static BlockDriver bdrv_sheepdog_unix = { .bdrv_co_create = sd_co_create, .bdrv_co_create_opts = sd_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, + .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1, .bdrv_getlength = sd_getlength, .bdrv_get_allocated_file_size = sd_get_allocated_file_size, .bdrv_co_truncate = sd_co_truncate, From 1656324ec0d1a9466160a5c5065b9bf11e6ea91b Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 24 Mar 2020 18:27:55 +0100 Subject: [PATCH 08/10] qemu-img: Fix check's leak/corruption fix report There are two problems with qemu-img check's report on how many leaks and/or corruptions have been fixed: (1) ImageCheck.has_leaks_fixed and ImageCheck.has_corruptions_fixed are only true when ImageCheck.leaks or ImageCheck.corruptions (respectively) are non-zero. qcow2's check implementation will set the latter to zero after it has fixed leaks and corruptions, though, so leaks-fixed and corruptions-fixed are actually never reported after successful repairs. We should always report them when they are non-zero, just like all the other fields of ImageCheck. (2) After something has been fixed and we run the check a second time, leaks_fixed and corruptions_fixed are taken from the first run; but has_leaks_fixed and has_corruptions_fixed are not. The second run actually cannot fix anything, so with (1) fixed, has_leaks_fixed and has_corruptions_fixed will always be false here. (With (1) unfixed, they will at least be false on successful runs, because then the number of leaks and corruptions found in the second run should be 0.) We should save has_leaks_fixed and has_corruptions_fixed just like we save leaks_fixed and corruptions_fixed. Signed-off-by: Max Reitz Reviewed-by: Eric Blake Message-Id: <20200324172757.1173824-2-mreitz@redhat.com> Signed-off-by: Max Reitz --- qemu-img.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index afddf33f08..b167376bd7 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -647,9 +647,9 @@ static int collect_image_check(BlockDriverState *bs, check->leaks = result.leaks; check->has_leaks = result.leaks != 0; check->corruptions_fixed = result.corruptions_fixed; - check->has_corruptions_fixed = result.corruptions != 0; + check->has_corruptions_fixed = result.corruptions_fixed != 0; check->leaks_fixed = result.leaks_fixed; - check->has_leaks_fixed = result.leaks != 0; + check->has_leaks_fixed = result.leaks_fixed != 0; check->image_end_offset = result.image_end_offset; check->has_image_end_offset = result.image_end_offset != 0; check->total_clusters = result.bfi.total_clusters; @@ -803,9 +803,12 @@ static int img_check(int argc, char **argv) if (check->corruptions_fixed || check->leaks_fixed) { int corruptions_fixed, leaks_fixed; + bool has_leaks_fixed, has_corruptions_fixed; leaks_fixed = check->leaks_fixed; + has_leaks_fixed = check->has_leaks_fixed; corruptions_fixed = check->corruptions_fixed; + has_corruptions_fixed = check->has_corruptions_fixed; if (output_format == OFORMAT_HUMAN) { qprintf(quiet, @@ -822,7 +825,9 @@ static int img_check(int argc, char **argv) ret = collect_image_check(bs, check, filename, fmt, 0); check->leaks_fixed = leaks_fixed; + check->has_leaks_fixed = has_leaks_fixed; check->corruptions_fixed = corruptions_fixed; + check->has_corruptions_fixed = has_corruptions_fixed; } if (!ret) { From 2f8bb28ff0123a4e84bd14facc541dd2159580c2 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 24 Mar 2020 18:27:56 +0100 Subject: [PATCH 09/10] iotests: Add poke_file_[bl]e functions Similarly to peek_file_[bl]e, we may want to write binary integers into a file. Currently, this often means messing around with poke_file and raw binary strings. I hope these functions make it a bit more comfortable. Signed-off-by: Max Reitz Code-suggested-by: Eric Blake Message-Id: <20200324172757.1173824-3-mreitz@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- tests/qemu-iotests/common.rc | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc index 4c246c0450..bf3b9fdea0 100644 --- a/tests/qemu-iotests/common.rc +++ b/tests/qemu-iotests/common.rc @@ -53,6 +53,30 @@ poke_file() printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null } +# poke_file_le $img_filename $offset $byte_width $value +# Example: poke_file_le "$TEST_IMG" 512 2 65534 +poke_file_le() +{ + local img=$1 ofs=$2 len=$3 val=$4 str='' + + while ((len--)); do + str+=$(printf '\\x%02x' $((val & 0xff))) + val=$((val >> 8)) + done + + poke_file "$img" "$ofs" "$str" +} + +# poke_file_be $img_filename $offset $byte_width $value +# Example: poke_file_be "$TEST_IMG" 512 2 65279 +poke_file_be() +{ + local img=$1 ofs=$2 len=$3 val=$4 + local str=$(printf "%0$((len * 2))x\n" $val | sed 's/\(..\)/\\x\1/g') + + poke_file "$img" "$ofs" "$str" +} + # peek_file_le 'test.img' 512 2 => 65534 peek_file_le() { From a507c51790fa955c1fccd4deca3c50476a862b83 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 24 Mar 2020 18:27:57 +0100 Subject: [PATCH 10/10] iotests/138: Test leaks/corruptions fixed report Test that qemu-img check reports the number of leaks and corruptions fixed in its JSON report (after a successful run). While touching the _unsupported_imgopts line, adjust the note on why data_file does not work with this test: The current comment sounds a bit like it is a mistake for qemu-img check not to check external data files' refcounts. But there are no such refcounts, so it is no mistake. Just say that qemu-img check does not do much for external data files, and this is why this test does not work with them. Signed-off-by: Max Reitz Message-Id: <20200324172757.1173824-4-mreitz@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- tests/qemu-iotests/138 | 41 ++++++++++++++++++++++++++++++++++++-- tests/qemu-iotests/138.out | 14 +++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138 index 54b01046ad..1d5b0bed6d 100755 --- a/tests/qemu-iotests/138 +++ b/tests/qemu-iotests/138 @@ -41,8 +41,10 @@ _supported_fmt qcow2 _supported_proto file _supported_os Linux # With an external data file, data clusters are not refcounted -# (and so qemu-img check does not check their refcount) -_unsupported_imgopts data_file +# (so qemu-img check would not do much); +# we want to modify the refcounts, so we need them to have a specific +# format (namely u16) +_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' echo echo '=== Check on an image with a multiple of 2^32 clusters ===' @@ -65,6 +67,41 @@ poke_file "$TEST_IMG" $((2048 + 8)) "\x00\x80\x00\x00\x00\x00\x00\x00" # allocate memory", we have an error showing that l2 entry is invalid. _check_test_img +echo +echo '=== Check leaks-fixed/corruptions-fixed report' +echo + +# After leaks and corruptions were fixed, those numbers should be +# reported by qemu-img check +_make_test_img 64k + +# Allocate data cluster +$QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io + +reftable_ofs=$(peek_file_be "$TEST_IMG" 48 8) +refblock_ofs=$(peek_file_be "$TEST_IMG" $reftable_ofs 8) + +# Introduce a leak: Make the image header's refcount 2 +poke_file_be "$TEST_IMG" "$refblock_ofs" 2 2 + +l1_ofs=$(peek_file_be "$TEST_IMG" 40 8) + +# Introduce a corruption: Drop the COPIED flag from the (first) L1 entry +l1_entry=$(peek_file_be "$TEST_IMG" $l1_ofs 8) +l1_entry=$((l1_entry & ~(1 << 63))) +poke_file_be "$TEST_IMG" $l1_ofs 8 $l1_entry + +echo +# Should print the number of corruptions and leaks fixed +# (Filter out all JSON fields (recognizable by their four-space +# indentation), but keep the "-fixed" fields (by removing two spaces +# from their indentation)) +# (Also filter out the L1 entry, because why not) +_check_test_img -r all --output=json \ + | sed -e 's/^ \(.*\)-fixed"/\1-fixed"/' \ + -e '/^ /d' \ + -e "s/\\([^0-9a-f]\\)$(printf %x $l1_entry)\\([^0-9a-f]\\)/\1L1_ENTRY_VALUE\2/" + # success, all done echo "*** done" rm -f $seq.full diff --git a/tests/qemu-iotests/138.out b/tests/qemu-iotests/138.out index aca7d47a80..79681e7cc9 100644 --- a/tests/qemu-iotests/138.out +++ b/tests/qemu-iotests/138.out @@ -9,4 +9,18 @@ ERROR: counting reference for region exceeding the end of the file by one cluste 1 errors were found on the image. Data may be corrupted, or further writes to the image may corrupt it. + +=== Check leaks-fixed/corruptions-fixed report + +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536 +wrote 65536/65536 bytes at offset 0 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +Leaked cluster 0 refcount=2 reference=1 +Repairing cluster 0 refcount=2 reference=1 +Repairing OFLAG_COPIED L2 cluster: l1_index=0 l1_entry=L1_ENTRY_VALUE refcount=1 +{ + "corruptions-fixed": 1, + "leaks-fixed": 1, +} *** done