:p
atchew
Login
The following changes since commit 67b6526cf042f22521feff5ea521a05d3dd2bf8f: Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into staging (2022-01-13 13:59:56 +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 e5e748739562268ef4063ee77bf53ad7040b25c7: iotests/testrunner.py: refactor test_field_width (2022-01-14 12:03:16 +0100) ---------------------------------------------------------------- Block layer patches - qemu-storage-daemon: Add vhost-user-blk help - block-backend: Fix use-after-free for BDS pointers after aio_poll() - qemu-img: Fix sparseness of output image with unaligned ranges - vvfat: Fix crashes in read-write mode - Fix device deletion events with -device JSON syntax - Code cleanups ---------------------------------------------------------------- Daniel P. Berrangé (1): softmmu: fix device deletion events with -device JSON syntax Emanuele Giuseppe Esposito (3): block_int: make bdrv_backing_overridden static include/sysemu/blockdev.h: remove drive_mark_claimed_by_board and inline drive_def include/sysemu/blockdev.h: remove drive_get_max_devs Hanna Reitz (2): iotests/stream-error-on-reset: New test iotests/308: Fix for CAP_DAC_OVERRIDE Kevin Wolf (3): vvfat: Fix size of temporary qcow file vvfat: Fix vvfat_write() for writes before the root directory iotests: Test qemu-img convert of zeroed data cluster Philippe Mathieu-Daudé (3): docs: Correct 'vhost-user-blk' spelling qemu-storage-daemon: Add vhost-user-blk help qapi/block: Restrict vhost-user-blk to CONFIG_VHOST_USER_BLK_SERVER Stefan Hajnoczi (1): block-backend: prevent dangling BDS pointers across aio_poll() Vladimir Sementsov-Ogievskiy (3): qemu-img: make is_allocated_sectors() more efficient block: drop BLK_PERM_GRAPH_MOD iotests/testrunner.py: refactor test_field_width qapi/block-core.json | 7 +- qapi/block-export.json | 6 +- qapi/qdev.json | 5 +- docs/tools/qemu-storage-daemon.rst | 2 +- include/block/block.h | 9 +- include/block/block_int.h | 3 - include/sysemu/blockdev.h | 3 - block.c | 11 +- block/block-backend.c | 19 ++- block/commit.c | 1 - block/mirror.c | 15 +-- block/monitor/block-hmp-cmds.c | 2 +- block/vvfat.c | 37 ++++-- blockdev.c | 24 +--- hw/block/block.c | 3 +- qemu-img.c | 23 +++- softmmu/vl.c | 8 +- storage-daemon/qemu-storage-daemon.c | 13 ++ tests/qtest/device-plug-test.c | 19 +++ scripts/render_block_graph.py | 1 - tests/qemu-iotests/testrunner.py | 21 ++-- tests/qemu-iotests/122 | 1 + tests/qemu-iotests/122.out | 2 + tests/qemu-iotests/273.out | 4 - tests/qemu-iotests/308 | 25 +++- tests/qemu-iotests/308.out | 2 +- tests/qemu-iotests/tests/stream-error-on-reset | 140 +++++++++++++++++++++ tests/qemu-iotests/tests/stream-error-on-reset.out | 5 + 28 files changed, 307 insertions(+), 104 deletions(-) create mode 100755 tests/qemu-iotests/tests/stream-error-on-reset create mode 100644 tests/qemu-iotests/tests/stream-error-on-reset.out
From: Emanuele Giuseppe Esposito <eesposit@redhat.com> bdrv_backing_overridden is only used in block.c, so there is no need to leave it in block_int.h Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20211215121140.456939-2-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/block_int.h | 3 --- block.c | 4 +++- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/include/block/block_int.h b/include/block/block_int.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -XXX,XX +XXX,XX @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size, void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix, QDict *options); -bool bdrv_backing_overridden(BlockDriverState *bs); - - /** * bdrv_add_aio_context_notifier: * diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, static void bdrv_reopen_commit(BDRVReopenState *reopen_state); static void bdrv_reopen_abort(BDRVReopenState *reopen_state); +static bool bdrv_backing_overridden(BlockDriverState *bs); + /* If non-zero, use only whitelisted block drivers */ static int use_bdrv_whitelist; @@ -XXX,XX +XXX,XX @@ static bool append_strong_runtime_options(QDict *d, BlockDriverState *bs) /* Note: This function may return false positives; it may return true * even if opening the backing file specified by bs's image header * would result in exactly bs->backing. */ -bool bdrv_backing_overridden(BlockDriverState *bs) +static bool bdrv_backing_overridden(BlockDriverState *bs) { if (bs->backing) { return strcmp(bs->auto_backing_file, -- 2.31.1
From: Emanuele Giuseppe Esposito <eesposit@redhat.com> drive_def is only a particular use case of qemu_opts_parse_noisily, so it can be inlined. Also remove drive_mark_claimed_by_board, as it is only defined but not implemented (nor used) anywhere. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20211215121140.456939-3-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/sysemu/blockdev.h | 2 -- block/monitor/block-hmp-cmds.c | 2 +- blockdev.c | 7 +------ softmmu/vl.c | 4 +++- 4 files changed, 5 insertions(+), 10 deletions(-) diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h index XXXXXXX..XXXXXXX 100644 --- a/include/sysemu/blockdev.h +++ b/include/sysemu/blockdev.h @@ -XXX,XX +XXX,XX @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo); void override_max_devs(BlockInterfaceType type, int max_devs); DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit); -void drive_mark_claimed_by_board(void); void drive_check_orphaned(void); DriveInfo *drive_get_by_index(BlockInterfaceType type, int index); int drive_get_max_bus(BlockInterfaceType type); int drive_get_max_devs(BlockInterfaceType type); -QemuOpts *drive_def(const char *optstr); QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file, const char *optstr); DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type, diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c index XXXXXXX..XXXXXXX 100644 --- a/block/monitor/block-hmp-cmds.c +++ b/block/monitor/block-hmp-cmds.c @@ -XXX,XX +XXX,XX @@ void hmp_drive_add(Monitor *mon, const QDict *qdict) return; } - opts = drive_def(optstr); + opts = qemu_opts_parse_noisily(qemu_find_opts("drive"), optstr, false); if (!opts) return; diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ static int drive_index_to_unit_id(BlockInterfaceType type, int index) return max_devs ? index % max_devs : index; } -QemuOpts *drive_def(const char *optstr) -{ - return qemu_opts_parse_noisily(qemu_find_opts("drive"), optstr, false); -} - QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file, const char *optstr) { QemuOpts *opts; - opts = drive_def(optstr); + opts = qemu_opts_parse_noisily(qemu_find_opts("drive"), optstr, false); if (!opts) { return NULL; } diff --git a/softmmu/vl.c b/softmmu/vl.c index XXXXXXX..XXXXXXX 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -XXX,XX +XXX,XX @@ void qemu_init(int argc, char **argv, char **envp) break; } case QEMU_OPTION_drive: - if (drive_def(optarg) == NULL) { + opts = qemu_opts_parse_noisily(qemu_find_opts("drive"), + optarg, false); + if (opts == NULL) { exit(1); } break; -- 2.31.1
From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Remove drive_get_max_devs, as it is not used by anyone. Last use was removed in commit 8f2d75e81d5 ("hw: Drop superfluous special checks for orphaned -drive"). Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20211215121140.456939-4-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/sysemu/blockdev.h | 1 - blockdev.c | 17 ----------------- 2 files changed, 18 deletions(-) diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h index XXXXXXX..XXXXXXX 100644 --- a/include/sysemu/blockdev.h +++ b/include/sysemu/blockdev.h @@ -XXX,XX +XXX,XX @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit); void drive_check_orphaned(void); DriveInfo *drive_get_by_index(BlockInterfaceType type, int index); int drive_get_max_bus(BlockInterfaceType type); -int drive_get_max_devs(BlockInterfaceType type); QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file, const char *optstr); diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ void blockdev_auto_del(BlockBackend *blk) } } -/** - * Returns the current mapping of how many units per bus - * a particular interface can support. - * - * A positive integer indicates n units per bus. - * 0 implies the mapping has not been established. - * -1 indicates an invalid BlockInterfaceType was given. - */ -int drive_get_max_devs(BlockInterfaceType type) -{ - if (type >= IF_IDE && type < IF_COUNT) { - return if_max_devs[type]; - } - - return -1; -} - static int drive_index_to_bus_id(BlockInterfaceType type, int index) { int max_devs = if_max_devs[type]; -- 2.31.1
From: Daniel P. Berrangé <berrange@redhat.com> The -device JSON syntax impl leaks a reference on the created DeviceState instance. As a result when you hot-unplug the device, the device_finalize method won't be called and thus it will fail to emit the required DEVICE_DELETED event. A 'json-cli' feature was previously added against the 'device_add' QMP command QAPI schema to indicated to mgmt apps that -device supported JSON syntax. Given the hotplug bug that feature flag is not usable for its purpose, so we add a new 'json-cli-hotplug' feature to indicate the -device supports JSON without breaking hotplug. Fixes: 5dacda5167560b3af8eadbce5814f60ba44b467e Resolves: https://gitlab.com/qemu-project/qemu/-/issues/802 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Message-Id: <20220105123847.4047954-2-berrange@redhat.com> Reviewed-by: Laurent Vivier <lvivier@redhat.com> Tested-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Thomas Huth <thuth@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/qdev.json | 5 ++++- softmmu/vl.c | 4 +++- tests/qtest/device-plug-test.c | 19 +++++++++++++++++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/qapi/qdev.json b/qapi/qdev.json index XXXXXXX..XXXXXXX 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -XXX,XX +XXX,XX @@ # @json-cli: If present, the "-device" command line option supports JSON # syntax with a structure identical to the arguments of this # command. +# @json-cli-hotplug: If present, the "-device" command line option supports JSON +# syntax without the reference counting leak that broke +# hot-unplug # # Notes: # @@ -XXX,XX +XXX,XX @@ { 'command': 'device_add', 'data': {'driver': 'str', '*bus': 'str', '*id': 'str'}, 'gen': false, # so we can get the additional arguments - 'features': ['json-cli'] } + 'features': ['json-cli', 'json-cli-hotplug'] } ## # @device_del: diff --git a/softmmu/vl.c b/softmmu/vl.c index XXXXXXX..XXXXXXX 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -XXX,XX +XXX,XX @@ static void qemu_create_cli_devices(void) qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, &error_fatal); QTAILQ_FOREACH(opt, &device_opts, next) { + DeviceState *dev; loc_push_restore(&opt->loc); /* * TODO Eventually we should call qmp_device_add() here to make sure it @@ -XXX,XX +XXX,XX @@ static void qemu_create_cli_devices(void) * from the start, so call qdev_device_add_from_qdict() directly for * now. */ - qdev_device_add_from_qdict(opt->opts, true, &error_fatal); + dev = qdev_device_add_from_qdict(opt->opts, true, &error_fatal); + object_unref(OBJECT(dev)); loc_pop(&opt->loc); } rom_reset_order_override(); diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/device-plug-test.c +++ b/tests/qtest/device-plug-test.c @@ -XXX,XX +XXX,XX @@ static void test_pci_unplug_request(void) qtest_quit(qtest); } +static void test_pci_unplug_json_request(void) +{ + QTestState *qtest = qtest_initf( + "-device '{\"driver\": \"virtio-mouse-pci\", \"id\": \"dev0\"}'"); + + /* + * Request device removal. As the guest is not running, the request won't + * be processed. However during system reset, the removal will be + * handled, removing the device. + */ + device_del(qtest, "dev0"); + system_reset(qtest); + wait_device_deleted_event(qtest, "dev0"); + + qtest_quit(qtest); +} + static void test_ccw_unplug(void) { QTestState *qtest = qtest_initf("-device virtio-balloon-ccw,id=dev0"); @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) */ qtest_add_func("/device-plug/pci-unplug-request", test_pci_unplug_request); + qtest_add_func("/device-plug/pci-unplug-json-request", + test_pci_unplug_json_request); if (!strcmp(arch, "s390x")) { qtest_add_func("/device-plug/ccw-unplug", -- 2.31.1
From: Philippe Mathieu-Daudé <f4bug@amsat.org> Reported-by: Eric Blake <eblake@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-Id: <20220107105420.395011-2-f4bug@amsat.org> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- docs/tools/qemu-storage-daemon.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst index XXXXXXX..XXXXXXX 100644 --- a/docs/tools/qemu-storage-daemon.rst +++ b/docs/tools/qemu-storage-daemon.rst @@ -XXX,XX +XXX,XX @@ Export raw image file ``disk.img`` over NBD UNIX domain socket ``nbd.sock``:: --nbd-server addr.type=unix,addr.path=nbd.sock \ --export type=nbd,id=export,node-name=disk,writable=on -Export a qcow2 image file ``disk.qcow2`` as a vhosts-user-blk device over UNIX +Export a qcow2 image file ``disk.qcow2`` as a vhost-user-blk device over UNIX domain socket ``vhost-user-blk.sock``:: $ qemu-storage-daemon \ -- 2.31.1
From: Philippe Mathieu-Daudé <philmd@redhat.com> Add missing vhost-user-blk help: $ qemu-storage-daemon -h ... --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>, addr.type=unix,addr.path=<socket-path>[,writable=on|off] [,logical-block-size=<block-size>][,num-queues=<num-queues>] export the specified block node as a vhosts-user-blk device over UNIX domain socket --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>, fd,addr.str=<fd>[,writable=on|off] [,logical-block-size=<block-size>][,num-queues=<num-queues>] export the specified block node as a vhosts-user-blk device over file descriptor ... Fixes: 90fc91d50b7 ("convert vhost-user-blk server to block export API") Reported-by: Qing Wang <qinwang@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-Id: <20220107105420.395011-3-f4bug@amsat.org> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- storage-daemon/qemu-storage-daemon.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c index XXXXXXX..XXXXXXX 100644 --- a/storage-daemon/qemu-storage-daemon.c +++ b/storage-daemon/qemu-storage-daemon.c @@ -XXX,XX +XXX,XX @@ static void help(void) " export the specified block node over FUSE\n" "\n" #endif /* CONFIG_FUSE */ +#ifdef CONFIG_VHOST_USER_BLK_SERVER +" --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>,\n" +" addr.type=unix,addr.path=<socket-path>[,writable=on|off]\n" +" [,logical-block-size=<block-size>][,num-queues=<num-queues>]\n" +" export the specified block node as a\n" +" vhost-user-blk device over UNIX domain socket\n" +" --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>,\n" +" fd,addr.str=<fd>[,writable=on|off]\n" +" [,logical-block-size=<block-size>][,num-queues=<num-queues>]\n" +" export the specified block node as a\n" +" vhost-user-blk device over file descriptor\n" +"\n" +#endif /* CONFIG_VHOST_USER_BLK_SERVER */ " --monitor [chardev=]name[,mode=control][,pretty[=on|off]]\n" " configure a QMP monitor\n" "\n" -- 2.31.1
From: Philippe Mathieu-Daudé <philmd@redhat.com> When building QEMU with --disable-vhost-user and using introspection, query-qmp-schema lists vhost-user-blk even though it's not actually available: { "execute": "query-qmp-schema" } { "return": [ ... { "name": "312", "members": [ { "name": "nbd" }, { "name": "vhost-user-blk" } ], "meta-type": "enum", "values": [ "nbd", "vhost-user-blk" ] }, Restrict vhost-user-blk in BlockExportType when CONFIG_VHOST_USER_BLK_SERVER is disabled, so it doesn't end listed by query-qmp-schema. Fixes: 90fc91d50b7 ("convert vhost-user-blk server to block export API") Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-Id: <20220107105420.395011-4-f4bug@amsat.org> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-export.json | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/qapi/block-export.json b/qapi/block-export.json index XXXXXXX..XXXXXXX 100644 --- a/qapi/block-export.json +++ b/qapi/block-export.json @@ -XXX,XX +XXX,XX @@ # Since: 4.2 ## { 'enum': 'BlockExportType', - 'data': [ 'nbd', 'vhost-user-blk', + 'data': [ 'nbd', + { 'name': 'vhost-user-blk', 'if': 'CONFIG_VHOST_USER_BLK_SERVER' }, { 'name': 'fuse', 'if': 'CONFIG_FUSE' } ] } ## @@ -XXX,XX +XXX,XX @@ 'discriminator': 'type', 'data': { 'nbd': 'BlockExportOptionsNbd', - 'vhost-user-blk': 'BlockExportOptionsVhostUserBlk', + 'vhost-user-blk': { 'type': 'BlockExportOptionsVhostUserBlk', + 'if': 'CONFIG_VHOST_USER_BLK_SERVER' }, 'fuse': { 'type': 'BlockExportOptionsFuse', 'if': 'CONFIG_FUSE' } } } -- 2.31.1
From: Stefan Hajnoczi <stefanha@redhat.com> The BlockBackend root child can change when aio_poll() is invoked. This happens when a temporary filter node is removed upon blockjob completion, for example. Functions in block/block-backend.c must be aware of this when using a blk_bs() pointer across aio_poll() because the BlockDriverState refcnt may reach 0, resulting in a stale pointer. One example is scsi_device_purge_requests(), which calls blk_drain() to wait for in-flight requests to cancel. If the backup blockjob is active, then the BlockBackend root child is a temporary filter BDS owned by the blockjob. The blockjob can complete during bdrv_drained_begin() and the last reference to the BDS is released when the temporary filter node is removed. This results in a use-after-free when blk_drain() calls bdrv_drained_end(bs) on the dangling pointer. Explicitly hold a reference to bs across block APIs that invoke aio_poll(). Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2021778 Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2036178 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20220111153613.25453-2-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/block-backend.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) 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 @@ BlockBackend *blk_by_public(BlockBackendPublic *public) void blk_remove_bs(BlockBackend *blk) { ThrottleGroupMember *tgm = &blk->public.throttle_group_member; - BlockDriverState *bs; BdrvChild *root; notifier_list_notify(&blk->remove_bs_notifiers, blk); if (tgm->throttle_state) { - bs = blk_bs(blk); + BlockDriverState *bs = blk_bs(blk); + + /* + * Take a ref in case blk_bs() changes across bdrv_drained_begin(), for + * example, if a temporary filter node is removed by a blockjob. + */ + bdrv_ref(bs); bdrv_drained_begin(bs); throttle_group_detach_aio_context(tgm); throttle_group_attach_aio_context(tgm, qemu_get_aio_context()); bdrv_drained_end(bs); + bdrv_unref(bs); } blk_update_root_state(blk); @@ -XXX,XX +XXX,XX @@ void blk_drain(BlockBackend *blk) BlockDriverState *bs = blk_bs(blk); if (bs) { + bdrv_ref(bs); bdrv_drained_begin(bs); } @@ -XXX,XX +XXX,XX @@ void blk_drain(BlockBackend *blk) if (bs) { bdrv_drained_end(bs); + bdrv_unref(bs); } } @@ -XXX,XX +XXX,XX @@ static int blk_do_set_aio_context(BlockBackend *blk, AioContext *new_context, int ret; if (bs) { + bdrv_ref(bs); + if (update_root_node) { ret = bdrv_child_try_set_aio_context(bs, new_context, blk->root, errp); if (ret < 0) { + bdrv_unref(bs); return ret; } } @@ -XXX,XX +XXX,XX @@ static int blk_do_set_aio_context(BlockBackend *blk, AioContext *new_context, throttle_group_attach_aio_context(tgm, new_context); bdrv_drained_end(bs); } + + bdrv_unref(bs); } blk->ctx = new_context; @@ -XXX,XX +XXX,XX @@ void blk_io_limits_disable(BlockBackend *blk) ThrottleGroupMember *tgm = &blk->public.throttle_group_member; assert(tgm->throttle_state); if (bs) { + bdrv_ref(bs); bdrv_drained_begin(bs); } throttle_group_unregister_tgm(tgm); if (bs) { bdrv_drained_end(bs); + bdrv_unref(bs); } } -- 2.31.1
From: Hanna Reitz <hreitz@redhat.com> Test the following scenario: - Simple stream block in two-layer backing chain (base and top) - The job is drained via blk_drain(), then an error occurs while the job settles the ongoing request - And so the job completes while in blk_drain() This was reported as a segfault, but is fixed by "block-backend: prevent dangling BDS pointers across aio_poll()". Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2036178 Signed-off-by: Hanna Reitz <hreitz@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20220111153613.25453-3-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- .../qemu-iotests/tests/stream-error-on-reset | 140 ++++++++++++++++++ .../tests/stream-error-on-reset.out | 5 + 2 files changed, 145 insertions(+) create mode 100755 tests/qemu-iotests/tests/stream-error-on-reset create mode 100644 tests/qemu-iotests/tests/stream-error-on-reset.out diff --git a/tests/qemu-iotests/tests/stream-error-on-reset b/tests/qemu-iotests/tests/stream-error-on-reset new file mode 100755 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/tests/stream-error-on-reset @@ -XXX,XX +XXX,XX @@ +#!/usr/bin/env python3 +# group: rw quick +# +# Test what happens when a stream job completes in a blk_drain(). +# +# Copyright (C) 2022 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/>. +# + +import os +import iotests +from iotests import imgfmt, qemu_img_create, qemu_io_silent, QMPTestCase + + +image_size = 1 * 1024 * 1024 +data_size = 64 * 1024 +base = os.path.join(iotests.test_dir, 'base.img') +top = os.path.join(iotests.test_dir, 'top.img') + + +# We want to test completing a stream job in a blk_drain(). +# +# The blk_drain() we are going to use is a virtio-scsi device resetting, +# which we can trigger by resetting the system. +# +# In order to have the block job complete on drain, we (1) throttle its +# base image so we can start the drain after it has begun, but before it +# completes, and (2) make it encounter an I/O error on the ensuing write. +# (If it completes regularly, the completion happens after the drain for +# some reason.) + +class TestStreamErrorOnReset(QMPTestCase): + def setUp(self) -> None: + """ + Create two images: + - base image {base} with {data_size} bytes allocated + - top image {top} without any data allocated + + And the following VM configuration: + - base image throttled to {data_size} + - top image with a blkdebug configuration so the first write access + to it will result in an error + - top image is attached to a virtio-scsi device + """ + assert qemu_img_create('-f', imgfmt, base, str(image_size)) == 0 + assert qemu_io_silent('-c', f'write 0 {data_size}', base) == 0 + assert qemu_img_create('-f', imgfmt, top, str(image_size)) == 0 + + self.vm = iotests.VM() + self.vm.add_args('-accel', 'tcg') # Make throttling work properly + self.vm.add_object(self.vm.qmp_to_opts({ + 'qom-type': 'throttle-group', + 'id': 'thrgr', + 'x-bps-total': str(data_size) + })) + self.vm.add_blockdev(self.vm.qmp_to_opts({ + 'driver': imgfmt, + 'node-name': 'base', + 'file': { + 'driver': 'throttle', + 'throttle-group': 'thrgr', + 'file': { + 'driver': 'file', + 'filename': base + } + } + })) + self.vm.add_blockdev(self.vm.qmp_to_opts({ + 'driver': imgfmt, + 'node-name': 'top', + 'file': { + 'driver': 'blkdebug', + 'node-name': 'top-blkdebug', + 'inject-error': [{ + 'event': 'pwritev', + 'immediately': 'true', + 'once': 'true' + }], + 'image': { + 'driver': 'file', + 'filename': top + } + }, + 'backing': 'base' + })) + self.vm.add_device(self.vm.qmp_to_opts({ + 'driver': 'virtio-scsi', + 'id': 'vscsi' + })) + self.vm.add_device(self.vm.qmp_to_opts({ + 'driver': 'scsi-hd', + 'bus': 'vscsi.0', + 'drive': 'top' + })) + self.vm.launch() + + def tearDown(self) -> None: + self.vm.shutdown() + os.remove(top) + os.remove(base) + + def test_stream_error_on_reset(self) -> None: + # Launch a stream job, which will take at least a second to + # complete, because the base image is throttled (so we can + # get in between it having started and it having completed) + res = self.vm.qmp('block-stream', job_id='stream', device='top') + self.assert_qmp(res, 'return', {}) + + while True: + ev = self.vm.event_wait('JOB_STATUS_CHANGE') + if ev['data']['status'] == 'running': + # Once the stream job is running, reset the system, which + # forces the virtio-scsi device to be reset, thus draining + # the stream job, and making it complete. Completing + # inside of that drain should not result in a segfault. + res = self.vm.qmp('system_reset') + self.assert_qmp(res, 'return', {}) + elif ev['data']['status'] == 'null': + # The test is done once the job is gone + break + + +if __name__ == '__main__': + # Passes with any format with backing file support, but qed and + # qcow1 do not seem to exercise the used-to-be problematic code + # path, so there is no point in having them in this list + iotests.main(supported_fmts=['qcow2', 'vmdk'], + supported_protocols=['file']) diff --git a/tests/qemu-iotests/tests/stream-error-on-reset.out b/tests/qemu-iotests/tests/stream-error-on-reset.out new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/tests/stream-error-on-reset.out @@ -XXX,XX +XXX,XX @@ +. +---------------------------------------------------------------------- +Ran 1 tests + +OK -- 2.31.1
From: Hanna Reitz <hreitz@redhat.com> With CAP_DAC_OVERRIDE (which e.g. root generally has), permission checks will be bypassed when opening files. 308 in one instance tries to open a read-only file (FUSE export) with qemu-io as read/write, and expects this to fail. However, when running it as root, opening will succeed (thanks to CAP_DAC_OVERRIDE) and only the actual write operation will fail. Note this as "Case not run", but have the test pass in either case. Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Fixes: 2c7dd057aa7bd7a875e9b1a53975c220d6380bc4 ("export/fuse: Pass default_permissions for mount") Signed-off-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20220103120014.13061-1-hreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/308 | 25 +++++++++++++++++++++++-- tests/qemu-iotests/308.out | 2 +- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/308 b/tests/qemu-iotests/308 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/308 +++ b/tests/qemu-iotests/308 @@ -XXX,XX +XXX,XX @@ echo '=== Writable export ===' fuse_export_add 'export-mp' "'mountpoint': '$EXT_MP', 'writable': true" # Check that writing to the read-only export fails -$QEMU_IO -f raw -c 'write -P 42 1M 64k' "$TEST_IMG" 2>&1 \ - | _filter_qemu_io | _filter_testdir | _filter_imgfmt +output=$($QEMU_IO -f raw -c 'write -P 42 1M 64k' "$TEST_IMG" 2>&1 \ + | _filter_qemu_io | _filter_testdir | _filter_imgfmt) + +# Expected reference output: Opening the file fails because it has no +# write permission +reference="Could not open 'TEST_DIR/t.IMGFMT': Permission denied" + +if echo "$output" | grep -q "$reference"; then + echo "Writing to read-only export failed: OK" +elif echo "$output" | grep -q "write failed: Permission denied"; then + # With CAP_DAC_OVERRIDE (e.g. when running this test as root), the export + # can be opened regardless of its file permissions, but writing will then + # fail. This is not the result for which we want to test, so count this as + # a SKIP. + _casenotrun "Opening RO export as R/W succeeded, perhaps because of" \ + "CAP_DAC_OVERRIDE" + + # Still, write this to the reference output to make the test pass + echo "Writing to read-only export failed: OK" +else + echo "Writing to read-only export failed: ERROR" + echo "$output" +fi # But here it should work $QEMU_IO -f raw -c 'write -P 42 1M 64k' "$EXT_MP" | _filter_qemu_io diff --git a/tests/qemu-iotests/308.out b/tests/qemu-iotests/308.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/308.out +++ b/tests/qemu-iotests/308.out @@ -XXX,XX +XXX,XX @@ virtual size: 0 B (0 bytes) 'mountpoint': 'TEST_DIR/t.IMGFMT.fuse', 'writable': true } } {"return": {}} -qemu-io: can't open device TEST_DIR/t.IMGFMT: Could not open 'TEST_DIR/t.IMGFMT': Permission denied +Writing to read-only export failed: OK wrote 65536/65536 bytes at offset 1048576 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 65536/65536 bytes at offset 1048576 -- 2.31.1
The size of the qcow size was calculated so that only the FAT partition would fit on it, but not the whole disk. However, offsets relative to the whole disk are used to access it, so increase its size to be large enough for that. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20211209151815.23495-1-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vvfat.c | 7 +++---- 1 file changed, 3 insertions(+), 4 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 vvfat_open(BlockDriverState *bs, QDict *options, int flags, dirname, cyls, heads, secs)); s->sector_count = cyls * heads * secs - s->offset_to_bootsector; + bs->total_sectors = cyls * heads * secs; if (qemu_opt_get_bool(opts, "rw", false)) { if (!bdrv_is_read_only(bs)) { @@ -XXX,XX +XXX,XX @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, } } - bs->total_sectors = cyls * heads * secs; - if (init_directories(s, dirname, heads, secs, errp)) { ret = -EIO; goto fail; @@ -XXX,XX +XXX,XX @@ static int enable_write_target(BlockDriverState *bs, Error **errp) } opts = qemu_opts_create(bdrv_qcow->create_opts, NULL, 0, &error_abort); - qemu_opt_set_number(opts, BLOCK_OPT_SIZE, s->sector_count * 512, - &error_abort); + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, + bs->total_sectors * BDRV_SECTOR_SIZE, &error_abort); qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, "fat:", &error_abort); ret = bdrv_create(bdrv_qcow, s->qcow_filename, opts, errp); -- 2.31.1
The calculation in sector2cluster() is done relative to the offset of the root directory. Any writes to blocks before the start of the root directory (in particular, writes to the FAT) result in negative values, which are not handled correctly in vvfat_write(). This changes sector2cluster() to return a signed value, and makes sure that vvfat_write() doesn't try to find mappings for negative cluster number. It clarifies the code in vvfat_write() to make it more obvious that the cluster numbers can be negative. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20211209152231.23756-1-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vvfat.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 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 read_directory(BDRVVVFATState* s, int mapping_index) return 0; } -static inline uint32_t sector2cluster(BDRVVVFATState* s,off_t sector_num) +static inline int32_t sector2cluster(BDRVVVFATState* s,off_t sector_num) { return (sector_num - s->offset_to_root_dir) / s->sectors_per_cluster; } @@ -XXX,XX +XXX,XX @@ static int vvfat_write(BlockDriverState *bs, int64_t sector_num, { BDRVVVFATState *s = bs->opaque; int i, ret; + int first_cluster, last_cluster; DLOG(checkpoint()); @@ -XXX,XX +XXX,XX @@ DLOG(checkpoint()); if (sector_num < s->offset_to_fat) return -1; - for (i = sector2cluster(s, sector_num); - i <= sector2cluster(s, sector_num + nb_sectors - 1);) { - mapping_t* mapping = find_mapping_for_cluster(s, i); + /* + * Values will be negative for writes to the FAT, which is located before + * the root directory. + */ + first_cluster = sector2cluster(s, sector_num); + last_cluster = sector2cluster(s, sector_num + nb_sectors - 1); + + for (i = first_cluster; i <= last_cluster;) { + mapping_t *mapping = NULL; + + if (i >= 0) { + mapping = find_mapping_for_cluster(s, i); + } + if (mapping) { if (mapping->read_only) { fprintf(stderr, "Tried to write to write-protected file %s\n", @@ -XXX,XX +XXX,XX @@ DLOG(checkpoint()); } } i = mapping->end; - } else + } else { i++; + } } /* @@ -XXX,XX +XXX,XX @@ DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sec return ret; } - for (i = sector2cluster(s, sector_num); - i <= sector2cluster(s, sector_num + nb_sectors - 1); i++) - if (i >= 0) + for (i = first_cluster; i <= last_cluster; i++) { + if (i >= 0) { s->used_clusters[i] |= USED_ALLOCATED; + } + } DLOG(checkpoint()); /* TODO: add timeout */ -- 2.31.1
This demonstrates what happens when the block status changes in sub-min_sparse granularity, but all of the parts are zeroed out. The alignment logic in is_allocated_sectors() prevents that the target image remains fully sparse as expected, but turns it into a data cluster of explicit zeros. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20211217164654.1184218-2-vsementsov@virtuozzo.com> Tested-by: Peter Lieven <pl@kamp.de> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/122 | 1 + tests/qemu-iotests/122.out | 10 ++++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/122 +++ b/tests/qemu-iotests/122 @@ -XXX,XX +XXX,XX @@ $QEMU_IO -c "write -P 0 0 64k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_test $QEMU_IO -c "write 0 1k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir $QEMU_IO -c "write 8k 1k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir $QEMU_IO -c "write 17k 1k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IO -c "write -P 0 65k 1k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir for min_sparse in 4k 8k; do echo diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/122.out +++ b/tests/qemu-iotests/122.out @@ -XXX,XX +XXX,XX @@ wrote 1024/1024 bytes at offset 8192 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 1024/1024 bytes at offset 17408 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 1024/1024 bytes at offset 66560 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) convert -S 4k [{ "start": 0, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}, @@ -XXX,XX +XXX,XX @@ convert -S 4k { "start": 8192, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}, { "start": 12288, "length": 4096, "depth": 0, "present": false, "zero": true, "data": false}, { "start": 16384, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}, -{ "start": 20480, "length": 67088384, "depth": 0, "present": false, "zero": true, "data": false}] +{ "start": 20480, "length": 46080, "depth": 0, "present": false, "zero": true, "data": false}, +{ "start": 66560, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}, +{ "start": 67584, "length": 67041280, "depth": 0, "present": false, "zero": true, "data": false}] convert -c -S 4k [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true}, @@ -XXX,XX +XXX,XX @@ convert -c -S 4k convert -S 8k [{ "start": 0, "length": 24576, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}, -{ "start": 24576, "length": 67084288, "depth": 0, "present": false, "zero": true, "data": false}] +{ "start": 24576, "length": 41984, "depth": 0, "present": false, "zero": true, "data": false}, +{ "start": 66560, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}, +{ "start": 67584, "length": 67041280, "depth": 0, "present": false, "zero": true, "data": false}] convert -c -S 8k [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true}, -- 2.31.1
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Consider the case when the whole buffer is zero and end is unaligned. If i <= tail, we return 1 and do one unaligned WRITE, RMW happens. If i > tail, we do on aligned WRITE_ZERO (or skip if target is zeroed) and again one unaligned WRITE, RMW happens. Let's do better: don't fragment the whole-zero buffer and report it as ZERO: in case of zeroed target we just do nothing and avoid RMW. If target is not zeroes, one unaligned WRITE_ZERO should not be much worse than one unaligned WRITE. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20211217164654.1184218-3-vsementsov@virtuozzo.com> Tested-by: Peter Lieven <pl@kamp.de> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qemu-img.c | 23 +++++++++++++++++++---- tests/qemu-iotests/122.out | 8 ++------ 2 files changed, 21 insertions(+), 10 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 int is_allocated_sectors(const uint8_t *buf, int n, int *pnum, } } + if (i == n) { + /* + * The whole buf is the same. + * No reason to split it into chunks, so return now. + */ + *pnum = i; + return !is_zero; + } + tail = (sector_num + i) & (alignment - 1); if (tail) { if (is_zero && i <= tail) { - /* treat unallocated areas which only consist - * of a small tail as allocated. */ + /* + * For sure next sector after i is data, and it will rewrite this + * tail anyway due to RMW. So, let's just write data now. + */ is_zero = false; } if (!is_zero) { - /* align up end offset of allocated areas. */ + /* If possible, align up end offset of allocated areas. */ i += alignment - tail; i = MIN(i, n); } else { - /* align down end offset of zero areas. */ + /* + * For sure next sector after i is data, and it will rewrite this + * tail anyway due to RMW. Better is avoid RMW and write zeroes up + * to aligned bound. + */ i -= tail; } } diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/122.out +++ b/tests/qemu-iotests/122.out @@ -XXX,XX +XXX,XX @@ convert -S 4k { "start": 8192, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}, { "start": 12288, "length": 4096, "depth": 0, "present": false, "zero": true, "data": false}, { "start": 16384, "length": 4096, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}, -{ "start": 20480, "length": 46080, "depth": 0, "present": false, "zero": true, "data": false}, -{ "start": 66560, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}, -{ "start": 67584, "length": 67041280, "depth": 0, "present": false, "zero": true, "data": false}] +{ "start": 20480, "length": 67088384, "depth": 0, "present": false, "zero": true, "data": false}] convert -c -S 4k [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true}, @@ -XXX,XX +XXX,XX @@ convert -c -S 4k convert -S 8k [{ "start": 0, "length": 24576, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}, -{ "start": 24576, "length": 41984, "depth": 0, "present": false, "zero": true, "data": false}, -{ "start": 66560, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}, -{ "start": 67584, "length": 67041280, "depth": 0, "present": false, "zero": true, "data": false}] +{ "start": 24576, "length": 67084288, "depth": 0, "present": false, "zero": true, "data": false}] convert -c -S 8k [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true}, -- 2.31.1
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> First, this permission never protected a node from being changed, as generic child-replacing functions don't check it. Second, it's a strange thing: it presents a permission of parent node to change its child. But generally, children are replaced by different mechanisms, like jobs or qmp commands, not by nodes. Graph-mod permission is hard to understand. All other permissions describe operations which done by parent node on its child: read, write, resize. Graph modification operations are something completely different. The only place where BLK_PERM_GRAPH_MOD is used as "perm" (not shared perm) is mirror_start_job, for s->target. Still modern code should use bdrv_freeze_backing_chain() to protect from graph modification, if we don't do it somewhere it may be considered as a bug. So, it's a bit risky to drop GRAPH_MOD, and analyzing of possible loss of protection is hard. But one day we should do it, let's do it now. One more bit of information is that locking the corresponding byte in file-posix doesn't make sense at all. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20210902093754.2352-1-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 7 ++----- include/block/block.h | 9 +++++---- block.c | 7 +------ block/commit.c | 1 - block/mirror.c | 15 +++------------ hw/block/block.c | 3 +-- scripts/render_block_graph.py | 1 - tests/qemu-iotests/273.out | 4 ---- 8 files changed, 12 insertions(+), 35 deletions(-) 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 @@ # # @resize: This permission is required to change the size of a block node. # -# @graph-mod: This permission is required to change the node that this -# BdrvChild points to. -# # Since: 4.0 ## { 'enum': 'BlockPermission', - 'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize', - 'graph-mod' ] } + 'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize' ] } + ## # @XDbgBlockGraphEdge: # 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 @@ enum { BLK_PERM_RESIZE = 0x08, /** - * This permission is required to change the node that this BdrvChild - * points to. + * There was a now-removed bit BLK_PERM_GRAPH_MOD, with value of 0x10. QEMU + * 6.1 and earlier may still lock the corresponding byte in block/file-posix + * locking. So, implementing some new permission should be very careful to + * not interfere with this old unused thing. */ - BLK_PERM_GRAPH_MOD = 0x10, - BLK_PERM_ALL = 0x1f, + BLK_PERM_ALL = 0x0f, DEFAULT_PERM_PASSTHROUGH = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ char *bdrv_perm_names(uint64_t perm) { BLK_PERM_WRITE, "write" }, { BLK_PERM_WRITE_UNCHANGED, "write unchanged" }, { BLK_PERM_RESIZE, "resize" }, - { BLK_PERM_GRAPH_MOD, "change children" }, { 0, NULL } }; @@ -XXX,XX +XXX,XX @@ static void bdrv_default_perms_for_cow(BlockDriverState *bs, BdrvChild *c, shared = 0; } - shared |= BLK_PERM_CONSISTENT_READ | BLK_PERM_GRAPH_MOD | - BLK_PERM_WRITE_UNCHANGED; + shared |= BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED; if (bs->open_flags & BDRV_O_INACTIVE) { shared |= BLK_PERM_WRITE | BLK_PERM_RESIZE; @@ -XXX,XX +XXX,XX @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm) [BLOCK_PERMISSION_WRITE] = BLK_PERM_WRITE, [BLOCK_PERMISSION_WRITE_UNCHANGED] = BLK_PERM_WRITE_UNCHANGED, [BLOCK_PERMISSION_RESIZE] = BLK_PERM_RESIZE, - [BLOCK_PERMISSION_GRAPH_MOD] = BLK_PERM_GRAPH_MOD, }; QEMU_BUILD_BUG_ON(ARRAY_SIZE(permissions) != BLOCK_PERMISSION__MAX); @@ -XXX,XX +XXX,XX @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, update_inherits_from = bdrv_inherits_from_recursive(base, explicit_top); /* success - we can delete the intermediate states, and link top->base */ - /* TODO Check graph modification op blockers (BLK_PERM_GRAPH_MOD) once - * we've figured out how they should work. */ if (!backing_file_str) { bdrv_refresh_filename(base); backing_file_str = base->filename; 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 @@ void commit_start(const char *job_id, BlockDriverState *bs, s->base = blk_new(s->common.job.aio_context, base_perms, BLK_PERM_CONSISTENT_READ - | BLK_PERM_GRAPH_MOD | BLK_PERM_WRITE_UNCHANGED); ret = blk_insert_bs(s->base, base, errp); if (ret < 0) { 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 void mirror_complete(Job *job, Error **errp) replace_aio_context = bdrv_get_aio_context(s->to_replace); aio_context_acquire(replace_aio_context); - /* TODO Translate this into permission system. Current definition of - * GRAPH_MOD would require to request it for the parents; they might - * not even be BlockDriverStates, however, so a BdrvChild can't address - * them. May need redefinition of GRAPH_MOD. */ + /* TODO Translate this into child freeze system. */ error_setg(&s->replace_blocker, "block device is in use by block-job-complete"); bdrv_op_block_all(s->to_replace, s->replace_blocker); @@ -XXX,XX +XXX,XX @@ static BlockJob *mirror_start_job( s = block_job_create(job_id, driver, NULL, mirror_top_bs, BLK_PERM_CONSISTENT_READ, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED | - BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed, + BLK_PERM_WRITE, speed, creation_flags, cb, opaque, errp); if (!s) { goto fail; @@ -XXX,XX +XXX,XX @@ static BlockJob *mirror_start_job( target_perms |= BLK_PERM_RESIZE; } - target_shared_perms |= BLK_PERM_CONSISTENT_READ - | BLK_PERM_WRITE - | BLK_PERM_GRAPH_MOD; + target_shared_perms |= BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE; } else if (bdrv_chain_contains(bs, bdrv_skip_filters(target))) { /* * We may want to allow this in the future, but it would @@ -XXX,XX +XXX,XX @@ static BlockJob *mirror_start_job( goto fail; } - if (backing_mode != MIRROR_LEAVE_BACKING_CHAIN) { - target_perms |= BLK_PERM_GRAPH_MOD; - } - s->target = blk_new(s->common.job.aio_context, target_perms, target_shared_perms); ret = blk_insert_bs(s->target, target, errp); diff --git a/hw/block/block.c b/hw/block/block.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/block.c +++ b/hw/block/block.c @@ -XXX,XX +XXX,XX @@ bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, perm |= BLK_PERM_WRITE; } - shared_perm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED | - BLK_PERM_GRAPH_MOD; + shared_perm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED; if (resizable) { shared_perm |= BLK_PERM_RESIZE; } diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py index XXXXXXX..XXXXXXX 100755 --- a/scripts/render_block_graph.py +++ b/scripts/render_block_graph.py @@ -XXX,XX +XXX,XX @@ def perm(arr): s = 'w' if 'write' in arr else '_' s += 'r' if 'consistent-read' in arr else '_' s += 'u' if 'write-unchanged' in arr else '_' - s += 'g' if 'graph-mod' in arr else '_' s += 's' if 'resize' in arr else '_' return s diff --git a/tests/qemu-iotests/273.out b/tests/qemu-iotests/273.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/273.out +++ b/tests/qemu-iotests/273.out @@ -XXX,XX +XXX,XX @@ Testing: -blockdev file,node-name=base,filename=TEST_DIR/t.IMGFMT.base -blockdev "name": "file", "parent": 5, "shared-perm": [ - "graph-mod", "write-unchanged", "consistent-read" ], @@ -XXX,XX +XXX,XX @@ Testing: -blockdev file,node-name=base,filename=TEST_DIR/t.IMGFMT.base -blockdev "name": "backing", "parent": 5, "shared-perm": [ - "graph-mod", "resize", "write-unchanged", "write", @@ -XXX,XX +XXX,XX @@ Testing: -blockdev file,node-name=base,filename=TEST_DIR/t.IMGFMT.base -blockdev "name": "file", "parent": 3, "shared-perm": [ - "graph-mod", "write-unchanged", "consistent-read" ], @@ -XXX,XX +XXX,XX @@ Testing: -blockdev file,node-name=base,filename=TEST_DIR/t.IMGFMT.base -blockdev "name": "backing", "parent": 3, "shared-perm": [ - "graph-mod", "resize", "write-unchanged", "write", -- 2.31.1
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> A lot of Optional[] types doesn't make code beautiful. test_field_width defaults to 8, but that is never used in the code. More over, if we want some default behavior for single call of test_run(), it should just print the whole test name, not limiting or expanding its width, so 8 is bad default. So, just drop the default as unused for now. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20211210201450.101576-1-vsementsov@virtuozzo.com> Reviewed-by: John Snow <jsnow@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/testrunner.py | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/testrunner.py +++ b/tests/qemu-iotests/testrunner.py @@ -XXX,XX +XXX,XX @@ def __enter__(self) -> 'TestRunner': def __exit__(self, exc_type: Any, exc_value: Any, traceback: Any) -> None: self._stack.close() - def test_print_one_line(self, test: str, starttime: str, + def test_print_one_line(self, test: str, + test_field_width: int, + starttime: str, endtime: Optional[str] = None, status: str = '...', lasttime: Optional[float] = None, thistime: Optional[float] = None, description: str = '', - test_field_width: Optional[int] = None, end: str = '\n') -> None: """ Print short test info before/after test run """ test = os.path.basename(test) - if test_field_width is None: - test_field_width = 8 - if self.makecheck and status != '...': if status and status != 'pass': status = f' [{status}]' @@ -XXX,XX +XXX,XX @@ def do_run_test(self, test: str, mp: bool) -> TestResult: casenotrun=casenotrun) def run_test(self, test: str, - test_field_width: Optional[int] = None, + test_field_width: int, mp: bool = False) -> TestResult: """ Run one test and print short status @@ -XXX,XX +XXX,XX @@ def run_test(self, test: str, if not self.makecheck: self.test_print_one_line(test=test, + test_field_width=test_field_width, status = 'started' if mp else '...', starttime=start, lasttime=last_el, - end = '\n' if mp else '\r', - test_field_width=test_field_width) + end = '\n' if mp else '\r') res = self.do_run_test(test, mp) end = datetime.datetime.now().strftime('%H:%M:%S') - self.test_print_one_line(test=test, status=res.status, + self.test_print_one_line(test=test, + test_field_width=test_field_width, + status=res.status, starttime=start, endtime=end, lasttime=last_el, thistime=res.elapsed, - description=res.description, - test_field_width=test_field_width) + description=res.description) if res.casenotrun: print(res.casenotrun) -- 2.31.1
The following changes since commit 13356edb87506c148b163b8c7eb0695647d00c2a: Merge tag 'block-pull-request' of https://gitlab.com/stefanha/qemu into staging (2023-01-24 09:45:33 +0000) are available in the Git repository at: https://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to d570177b50c389f379f93183155a27d44856ab46: qemu-img: Change info key names for protocol nodes (2023-02-01 16:52:33 +0100) v4: - Fixed the 'qemu-img-close-errors' test case to run only on Linux and only with the file protocol, use qemu-io instead of truncate v3: - Make the compiler happier on BSD and CentOS Stream 8 v2: - Rebased to resolve merge conflicts in coroutine.h ---------------------------------------------------------------- Block layer patches - qemu-img info: Show protocol-level information - Move more functions to coroutines - Make coroutine annotations ready for static analysis - qemu-img: Fix exit code for errors closing the image - qcow2 bitmaps: Fix theoretical corruption in error path - pflash: Only load non-zero parts of backend image to save memory - Code cleanup and test case improvements ---------------------------------------------------------------- Alberto Faria (2): coroutine: annotate coroutine_fn for libclang block: Add no_coroutine_fn and coroutine_mixed_fn marker Emanuele Giuseppe Esposito (14): block-coroutine-wrapper: support void functions block: Convert bdrv_io_plug() to co_wrapper block: Convert bdrv_io_unplug() to co_wrapper block: Convert bdrv_is_inserted() to co_wrapper block: Rename refresh_total_sectors to bdrv_refresh_total_sectors block: Convert bdrv_refresh_total_sectors() to co_wrapper_mixed block-backend: use bdrv_getlength instead of blk_getlength block: use bdrv_co_refresh_total_sectors when possible block: Convert bdrv_get_allocated_file_size() to co_wrapper block: Convert bdrv_get_info() to co_wrapper_mixed block: Convert bdrv_eject() to co_wrapper block: Convert bdrv_lock_medium() to co_wrapper block: Convert bdrv_debug_event() to co_wrapper_mixed block: Rename bdrv_load/save_vmstate() to bdrv_co_load/save_vmstate() Hanna Reitz (12): block: Improve empty format-specific info dump block/file: Add file-specific image info block/vmdk: Change extent info type block: Split BlockNodeInfo off of ImageInfo qemu-img: Use BlockNodeInfo block/qapi: Let bdrv_query_image_info() recurse block/qapi: Introduce BlockGraphInfo block/qapi: Add indentation to bdrv_node_info_dump() iotests: Filter child node information iotests/106, 214, 308: Read only one size line qemu-img: Let info print block graph qemu-img: Change info key names for protocol nodes Kevin Wolf (4): qcow2: Fix theoretical corruption in store_bitmap() error path qemu-img commit: Report errors while closing the image qemu-img bitmap: Report errors while closing the image qemu-iotests: Test qemu-img bitmap/commit exit code on error Paolo Bonzini (2): qemu-io: do not reinvent the blk_pwrite_zeroes wheel block: remove bdrv_coroutine_enter Philippe Mathieu-Daudé (1): block/nbd: Add missing <qemu/bswap.h> include Thomas Huth (2): tests/qemu-iotests/312: Mark "quorum" as required driver tests/qemu-iotests/262: Check for availability of "blkverify" first Xiang Zheng (1): pflash: Only read non-zero parts of backend image qapi/block-core.json | 123 +++++++- include/block/block-common.h | 11 +- include/block/block-io.h | 41 ++- include/block/block_int-common.h | 26 +- include/block/block_int-io.h | 5 +- include/block/nbd.h | 1 + include/block/qapi.h | 14 +- include/qemu/osdep.h | 44 +++ include/sysemu/block-backend-io.h | 31 +- block.c | 88 +++--- block/blkdebug.c | 11 +- block/blkio.c | 15 +- block/blklogwrites.c | 6 +- block/blkreplay.c | 6 +- block/blkverify.c | 6 +- block/block-backend.c | 38 +-- block/commit.c | 4 +- block/copy-on-read.c | 18 +- block/crypto.c | 14 +- block/curl.c | 10 +- block/file-posix.c | 137 +++++---- block/file-win32.c | 18 +- block/filter-compress.c | 20 +- block/gluster.c | 23 +- block/io.c | 76 ++--- block/iscsi.c | 17 +- block/mirror.c | 6 +- block/monitor/block-hmp-cmds.c | 2 +- block/nbd.c | 8 +- block/nfs.c | 4 +- block/null.c | 13 +- block/nvme.c | 14 +- block/preallocate.c | 16 +- block/qapi.c | 317 ++++++++++++++++----- block/qcow.c | 5 +- block/qcow2-bitmap.c | 5 +- block/qcow2-refcount.c | 2 +- block/qcow2.c | 17 +- block/qed.c | 11 +- block/quorum.c | 8 +- block/raw-format.c | 25 +- block/rbd.c | 9 +- block/replication.c | 6 +- block/ssh.c | 4 +- block/throttle.c | 6 +- block/vdi.c | 7 +- block/vhdx.c | 5 +- block/vmdk.c | 22 +- block/vpc.c | 5 +- blockdev.c | 8 +- hw/block/block.c | 36 ++- hw/scsi/scsi-disk.c | 5 + qemu-img.c | 100 +++++-- qemu-io-cmds.c | 62 +--- tests/unit/test-block-iothread.c | 3 + scripts/block-coroutine-wrapper.py | 20 +- tests/qemu-iotests/iotests.py | 18 +- block/meson.build | 1 + tests/qemu-iotests/065 | 2 +- tests/qemu-iotests/106 | 4 +- tests/qemu-iotests/214 | 6 +- tests/qemu-iotests/262 | 3 +- tests/qemu-iotests/302.out | 5 + tests/qemu-iotests/308 | 4 +- tests/qemu-iotests/312 | 1 + tests/qemu-iotests/common.filter | 22 +- tests/qemu-iotests/common.rc | 22 +- tests/qemu-iotests/tests/qemu-img-close-errors | 96 +++++++ tests/qemu-iotests/tests/qemu-img-close-errors.out | 23 ++ 69 files changed, 1209 insertions(+), 552 deletions(-) create mode 100755 tests/qemu-iotests/tests/qemu-img-close-errors create mode 100644 tests/qemu-iotests/tests/qemu-img-close-errors.out