From bb9f8dd0e15a9744b8d09d06ecb6a18ca3dcc173 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Tue, 6 Sep 2016 15:26:37 +0100 Subject: [PATCH 01/33] qcow2: fix encryption during cow of sectors Broken in previous commit: commit aaa4d20b4972bb1a811ce929502e6741835d584e Author: Kevin Wolf Date: Wed Jun 1 15:21:05 2016 +0200 qcow2: Make copy_sectors() byte based The copy_sectors() code was originally using the 'sector' parameter for encryption, which was passed in by the caller from the QCowL2Meta.offset field (aka the guest logical offset). After the change, the code is using 'cluster_offset' which was passed in from QCow2L2Meta.alloc_offset field (aka the host physical offset). This would cause the data to be encrypted using an incorrect initialization vector which will in turn cause later reads to return garbage. Although current qcow2 built-in encryption is blocked from usage in the emulator, one could still hit this if writing to the file via qemu-{img,io,nbd} commands. Cc: qemu-stable@nongnu.org Signed-off-by: Daniel P. Berrange Signed-off-by: Kevin Wolf --- block/qcow2-cluster.c | 2 +- tests/qemu-iotests/158 | 80 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/158.out | 36 +++++++++++++++++ tests/qemu-iotests/group | 1 + 4 files changed, 118 insertions(+), 1 deletion(-) create mode 100755 tests/qemu-iotests/158 create mode 100644 tests/qemu-iotests/158.out diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 9ab445dd17..61d1ffd223 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -429,7 +429,7 @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs, if (bs->encrypted) { Error *err = NULL; - int64_t sector = (cluster_offset + offset_in_cluster) + int64_t sector = (src_cluster_offset + offset_in_cluster) >> BDRV_SECTOR_BITS; assert(s->cipher); assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0); diff --git a/tests/qemu-iotests/158 b/tests/qemu-iotests/158 new file mode 100755 index 0000000000..a6cdd6d8cf --- /dev/null +++ b/tests/qemu-iotests/158 @@ -0,0 +1,80 @@ +#!/bin/bash +# +# Test encrypted read/write using backing files +# +# Copyright (C) 2015 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 . +# + +# creator +owner=berrange@redhat.com + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default! + +_cleanup() +{ + _cleanup_test_img +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt qcow2 +_supported_proto generic +_supported_os Linux + + +size=128M +TEST_IMG_BASE=$TEST_IMG.base + +TEST_IMG_SAVE=$TEST_IMG +TEST_IMG=$TEST_IMG_BASE +echo "== create base ==" +IMGOPTS="encryption=on" _make_test_img $size +TEST_IMG=$TEST_IMG_SAVE + +echo +echo "== writing whole image ==" +echo "astrochicken" | $QEMU_IO -c "write -P 0xa 0 $size" "$TEST_IMG_BASE" | _filter_qemu_io | _filter_testdir + +echo +echo "== verify pattern ==" +echo "astrochicken" | $QEMU_IO -c "read -P 0xa 0 $size" "$TEST_IMG_BASE" | _filter_qemu_io | _filter_testdir + +echo "== create overlay ==" +IMGOPTS="encryption=on" _make_test_img -b "$TEST_IMG_BASE" $size + +echo +echo "== writing part of a cluster ==" +echo "astrochicken" | $QEMU_IO -c "write -P 0xe 0 1024" "$TEST_IMG" | _filter_qemu_io | _filter_testdir + +echo +echo "== verify pattern ==" +echo "astrochicken" | $QEMU_IO -c "read -P 0xe 0 1024" "$TEST_IMG" | _filter_qemu_io | _filter_testdir +echo +echo "== verify pattern ==" +echo "astrochicken" | $QEMU_IO -c "read -P 0xa 1024 64512" "$TEST_IMG" | _filter_qemu_io | _filter_testdir + + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/158.out b/tests/qemu-iotests/158.out new file mode 100644 index 0000000000..b3f37e28ef --- /dev/null +++ b/tests/qemu-iotests/158.out @@ -0,0 +1,36 @@ +QA output created by 158 +== create base == +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728 encryption=on + +== writing whole image == +Disk image 'TEST_DIR/t.qcow2.base' is encrypted. +password: +wrote 134217728/134217728 bytes at offset 0 +128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== verify pattern == +Disk image 'TEST_DIR/t.qcow2.base' is encrypted. +password: +read 134217728/134217728 bytes at offset 0 +128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +== create overlay == +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base encryption=on + +== writing part of a cluster == +Disk image 'TEST_DIR/t.qcow2' is encrypted. +password: +wrote 1024/1024 bytes at offset 0 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== verify pattern == +Disk image 'TEST_DIR/t.qcow2' is encrypted. +password: +read 1024/1024 bytes at offset 0 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== verify pattern == +Disk image 'TEST_DIR/t.qcow2' is encrypted. +password: +read 64512/64512 bytes at offset 1024 +63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index a57fc9218f..7eb17707a2 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -157,6 +157,7 @@ 155 rw auto 156 rw auto quick 157 auto +158 rw auto quick 159 rw auto quick 160 rw auto quick 162 auto quick From 1813d33015f0a6780609739999f3962f2dc56921 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 15 Sep 2016 17:44:16 +0200 Subject: [PATCH 02/33] hmp: Remove dead code in hmp_qemu_io() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit blk can never be NULL, drop the check. This fixes a Coverity warning. Signed-off-by: Kevin Wolf Reviewed-by: Marc-André Lureau --- hmp.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/hmp.c b/hmp.c index ad33b442d2..0a16aefc2a 100644 --- a/hmp.c +++ b/hmp.c @@ -1924,6 +1924,7 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict) { BlockBackend *blk; BlockBackend *local_blk = NULL; + AioContext *aio_context; const char* device = qdict_get_str(qdict, "device"); const char* command = qdict_get_str(qdict, "command"); Error *err = NULL; @@ -1939,17 +1940,12 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict) } } - if (blk) { - AioContext *aio_context = blk_get_aio_context(blk); - aio_context_acquire(aio_context); + aio_context = blk_get_aio_context(blk); + aio_context_acquire(aio_context); - qemuio_command(blk, command); + qemuio_command(blk, command); - aio_context_release(aio_context); - } else { - error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); - } + aio_context_release(aio_context); fail: blk_unref(local_blk); From c2519009b428fb19a9fdbc707e786b1cebd4994f Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 15 Sep 2016 19:42:49 +0300 Subject: [PATCH 03/33] tests: allow to specify list of formats to test for check-block.sh This would make code better and allow to test specific format. Signed-off-by: Denis V. Lunev CC: Stefan Hajnoczi CC: Kevin Wolf CC: Paolo Bonzini Signed-off-by: Kevin Wolf --- tests/check-block.sh | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/check-block.sh b/tests/check-block.sh index a37797a494..c3de3789c4 100755 --- a/tests/check-block.sh +++ b/tests/check-block.sh @@ -1,5 +1,10 @@ #!/bin/sh +FORMAT_LIST="raw qcow2 qed vmdk vpc" +if [ "$#" -ne 0 ]; then + FORMAT_LIST="$@" +fi + export QEMU_PROG="$(pwd)/x86_64-softmmu/qemu-system-x86_64" export QEMU_IMG_PROG="$(pwd)/qemu-img" export QEMU_IO_PROG="$(pwd)/qemu-io" @@ -12,10 +17,8 @@ fi cd tests/qemu-iotests ret=0 -./check -T -nocache -raw || ret=1 -./check -T -nocache -qcow2 || ret=1 -./check -T -nocache -qed|| ret=1 -./check -T -nocache -vmdk|| ret=1 -./check -T -nocache -vpc || ret=1 +for FMT in $FORMAT_LIST ; do + ./check -T -nocache -$FMT || ret=1 +done exit $ret From 38b5e4c3dc6d713eae340341ee139c12d5c1a21e Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Thu, 15 Sep 2016 17:52:59 +0300 Subject: [PATCH 04/33] block: Remove bdrv_is_snapshot This is unnecessary and has been unused since 5433c24f0f9306c82ad9bcc. Signed-off-by: Alberto Garcia Reviewed-by: Kevin Wolf Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block.c | 5 ----- include/block/block.h | 1 - 2 files changed, 6 deletions(-) diff --git a/block.c b/block.c index afaff93423..d448520951 100644 --- a/block.c +++ b/block.c @@ -3013,11 +3013,6 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag) return false; } -int bdrv_is_snapshot(BlockDriverState *bs) -{ - return !!(bs->open_flags & BDRV_O_SNAPSHOT); -} - /* backing_file can either be relative, or absolute, or a protocol. If it is * relative, it must be relative to the chain. So, passing in bs->filename * from a BDS as backing_file should not be done, as that may be relative to diff --git a/include/block/block.h b/include/block/block.h index ffecebfb3a..f374e1a725 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -414,7 +414,6 @@ void bdrv_get_full_backing_filename_from_filename(const char *backed, const char *backing, char *dest, size_t sz, Error **errp); -int bdrv_is_snapshot(BlockDriverState *bs); int path_has_protocol(const char *path); int path_is_absolute(const char *path); From 14499ea5413be45bbb3934dd6fd8fa27c54c1dd4 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Thu, 15 Sep 2016 17:53:00 +0300 Subject: [PATCH 05/33] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags If an image is opened with snapshot=on, its flags are modified by bdrv_backing_options() and then bs->open_flags is updated accordingly. This last step is unnecessary if we calculate the new flags before setting bs->open_flags. Soon we'll introduce the "read-only" option, and then we'll need to be able to modify its value in the QDict when snapshot=on. This is more cumbersome if bs->options is already set. This patch simplifies that. Other than that, there are no semantic changes. Although it might seem that bs->options can have a different value now because it is stored after calling bdrv_backing_options(), this call doesn't actually modify them in this scenario. The code that sets BDRV_O_ALLOW_RDWR is also moved for the same reason. Signed-off-by: Alberto Garcia Signed-off-by: Kevin Wolf --- block.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/block.c b/block.c index d448520951..324dbe702b 100644 --- a/block.c +++ b/block.c @@ -1675,6 +1675,17 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, goto fail; } + if (flags & BDRV_O_RDWR) { + flags |= BDRV_O_ALLOW_RDWR; + } + + if (flags & BDRV_O_SNAPSHOT) { + snapshot_options = qdict_new(); + bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options, + flags, options); + bdrv_backing_options(&flags, options, flags, options); + } + bs->open_flags = flags; bs->options = options; options = qdict_clone_shallow(options); @@ -1699,18 +1710,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, /* Open image file without format layer */ if ((flags & BDRV_O_PROTOCOL) == 0) { - if (flags & BDRV_O_RDWR) { - flags |= BDRV_O_ALLOW_RDWR; - } - if (flags & BDRV_O_SNAPSHOT) { - snapshot_options = qdict_new(); - bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options, - flags, options); - bdrv_backing_options(&flags, options, flags, options); - } - - bs->open_flags = flags; - file = bdrv_open_child(filename, options, "file", bs, &child_file, true, &local_err); if (local_err) { From 9b7e8691670fab57c387b6955dc14a09696ae034 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Thu, 15 Sep 2016 17:53:01 +0300 Subject: [PATCH 06/33] block: Update bs->open_flags earlier in bdrv_open_common() We're only doing this immediately before opening the image, but bs->open_flags is used earlier in the function. At the moment this is not causing problems because none of the checked flags are modified by update_flags_from_options(), but this will change when we introduce the "read-only" option. This patch calls update_flags_from_options() at the beginning of the function, immediately after creating the QemuOpts. Signed-off-by: Alberto Garcia Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index 324dbe702b..a98b75e4df 100644 --- a/block.c +++ b/block.c @@ -961,6 +961,8 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file, goto fail_opts; } + update_flags_from_options(&bs->open_flags, opts); + driver_name = qemu_opt_get(opts, "driver"); drv = bdrv_find_format(driver_name); assert(drv != NULL); @@ -1022,9 +1024,6 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file, bs->drv = drv; bs->opaque = g_malloc0(drv->instance_size); - /* Apply cache mode options */ - update_flags_from_options(&bs->open_flags, opts); - /* Open the image, either directly or using a protocol */ open_flags = bdrv_open_flags(bs, bs->open_flags); if (drv->bdrv_file_open) { From f87a0e29a9814a7ab6ee92b238989fed6186c4f3 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Thu, 15 Sep 2016 17:53:02 +0300 Subject: [PATCH 07/33] block: Add "read-only" to the options QDict This adds the "read-only" option to the QDict. One important effect of this change is that when a child inherits options from its parent, the existing "read-only" mode can be preserved if it was explicitly set previously. This addresses scenarios like this: [E] <- [D] <- [C] <- [B] <- [A] In this case, if we reopen [D] with read-only=off, and later reopen [B], then [D] will not inherit read-only=on from its parent during the bdrv_reopen_queue_child() stage. The BDRV_O_RDWR flag is not removed yet, but its keep in sync with the value of the "read-only" option. Signed-off-by: Alberto Garcia Signed-off-by: Kevin Wolf --- block.c | 38 +++++++++++++++++++++++++++++++++++--- block/vvfat.c | 3 ++- blockdev.c | 16 +++++++--------- include/block/block.h | 1 + 4 files changed, 45 insertions(+), 13 deletions(-) diff --git a/block.c b/block.c index a98b75e4df..b724adacfc 100644 --- a/block.c +++ b/block.c @@ -733,6 +733,9 @@ static void bdrv_temp_snapshot_options(int *child_flags, QDict *child_options, qdict_set_default_str(child_options, BDRV_OPT_CACHE_DIRECT, "off"); qdict_set_default_str(child_options, BDRV_OPT_CACHE_NO_FLUSH, "on"); + /* Copy the read-only option from the parent */ + qdict_copy_default(child_options, parent_options, BDRV_OPT_READ_ONLY); + /* aio=native doesn't work for cache.direct=off, so disable it for the * temporary snapshot */ *child_flags &= ~BDRV_O_NATIVE_AIO; @@ -755,6 +758,9 @@ static void bdrv_inherited_options(int *child_flags, QDict *child_options, qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT); qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH); + /* Inherit the read-only option from the parent if it's not set */ + qdict_copy_default(child_options, parent_options, BDRV_OPT_READ_ONLY); + /* Our block drivers take care to send flushes and respect unmap policy, * so we can default to enable both on lower layers regardless of the * corresponding parent options. */ @@ -808,7 +814,8 @@ static void bdrv_backing_options(int *child_flags, QDict *child_options, qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH); /* backing files always opened read-only */ - flags &= ~(BDRV_O_RDWR | BDRV_O_COPY_ON_READ); + qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "on"); + flags &= ~BDRV_O_COPY_ON_READ; /* snapshot=on is handled on the top layer */ flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_TEMPORARY); @@ -855,6 +862,14 @@ static void update_flags_from_options(int *flags, QemuOpts *opts) if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_DIRECT, false)) { *flags |= BDRV_O_NOCACHE; } + + *flags &= ~BDRV_O_RDWR; + + assert(qemu_opt_find(opts, BDRV_OPT_READ_ONLY)); + if (!qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false)) { + *flags |= BDRV_O_RDWR; + } + } static void update_options_from_flags(QDict *options, int flags) @@ -867,6 +882,10 @@ static void update_options_from_flags(QDict *options, int flags) qdict_put(options, BDRV_OPT_CACHE_NO_FLUSH, qbool_from_bool(flags & BDRV_O_NO_FLUSH)); } + if (!qdict_haskey(options, BDRV_OPT_READ_ONLY)) { + qdict_put(options, BDRV_OPT_READ_ONLY, + qbool_from_bool(!(flags & BDRV_O_RDWR))); + } } static void bdrv_assign_node_name(BlockDriverState *bs, @@ -930,6 +949,11 @@ static QemuOptsList bdrv_runtime_opts = { .type = QEMU_OPT_BOOL, .help = "Ignore flush requests", }, + { + .name = BDRV_OPT_READ_ONLY, + .type = QEMU_OPT_BOOL, + .help = "Node is opened in read-only mode", + }, { /* end of list */ } }, }; @@ -1674,14 +1698,22 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, goto fail; } - if (flags & BDRV_O_RDWR) { - flags |= BDRV_O_ALLOW_RDWR; + /* Set the BDRV_O_RDWR and BDRV_O_ALLOW_RDWR flags. + * FIXME: we're parsing the QDict to avoid having to create a + * QemuOpts just for this, but neither option is optimal. */ + if (g_strcmp0(qdict_get_try_str(options, BDRV_OPT_READ_ONLY), "on") && + !qdict_get_try_bool(options, BDRV_OPT_READ_ONLY, false)) { + flags |= (BDRV_O_RDWR | BDRV_O_ALLOW_RDWR); + } else { + flags &= ~BDRV_O_RDWR; } if (flags & BDRV_O_SNAPSHOT) { snapshot_options = qdict_new(); bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options, flags, options); + /* Let bdrv_backing_options() override "read-only" */ + qdict_del(options, BDRV_OPT_READ_ONLY); bdrv_backing_options(&flags, options, flags, options); } diff --git a/block/vvfat.c b/block/vvfat.c index ba2620f3c2..ded21092ee 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -2971,7 +2971,8 @@ static BlockDriver vvfat_write_target = { static void vvfat_qcow_options(int *child_flags, QDict *child_options, int parent_flags, QDict *parent_options) { - *child_flags = BDRV_O_RDWR | BDRV_O_NO_FLUSH; + qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "off"); + *child_flags = BDRV_O_NO_FLUSH; } static const BdrvChildRole child_vvfat_qcow = { diff --git a/blockdev.c b/blockdev.c index 301039392c..c3f8da4700 100644 --- a/blockdev.c +++ b/blockdev.c @@ -360,9 +360,6 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags, const char *aio; if (bdrv_flags) { - if (!qemu_opt_get_bool(opts, "read-only", false)) { - *bdrv_flags |= BDRV_O_RDWR; - } if (qemu_opt_get_bool(opts, "copy-on-read", false)) { *bdrv_flags |= BDRV_O_COPY_ON_READ; } @@ -471,7 +468,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, int bdrv_flags = 0; int on_read_error, on_write_error; bool account_invalid, account_failed; - bool writethrough; + bool writethrough, read_only; BlockBackend *blk; BlockDriverState *bs; ThrottleConfig cfg; @@ -567,6 +564,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, bdrv_flags |= BDRV_O_SNAPSHOT; } + read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false); + /* init */ if ((!file || !*file) && !qdict_size(bs_opts)) { BlockBackendRootState *blk_rs; @@ -574,7 +573,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, blk = blk_new(); blk_rs = blk_get_root_state(blk); blk_rs->open_flags = bdrv_flags; - blk_rs->read_only = !(bdrv_flags & BDRV_O_RDWR); + blk_rs->read_only = read_only; blk_rs->detect_zeroes = detect_zeroes; QDECREF(bs_opts); @@ -588,6 +587,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, * Apply the defaults here instead. */ qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_DIRECT, "off"); qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_NO_FLUSH, "off"); + qdict_set_default_str(bs_opts, BDRV_OPT_READ_ONLY, + read_only ? "on" : "off"); assert((bdrv_flags & BDRV_O_CACHE_MASK) == 0); if (runstate_check(RUN_STATE_INMIGRATE)) { @@ -682,6 +683,7 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp) * Apply the defaults here instead. */ qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_DIRECT, "off"); qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_NO_FLUSH, "off"); + qdict_set_default_str(bs_opts, BDRV_OPT_READ_ONLY, "off"); if (runstate_check(RUN_STATE_INMIGRATE)) { bdrv_flags |= BDRV_O_INACTIVE; @@ -4158,10 +4160,6 @@ static QemuOptsList qemu_root_bds_opts = { .name = "aio", .type = QEMU_OPT_STRING, .help = "host AIO implementation (threads, native)", - },{ - .name = "read-only", - .type = QEMU_OPT_BOOL, - .help = "open drive file as read-only", },{ .name = "copy-on-read", .type = QEMU_OPT_BOOL, diff --git a/include/block/block.h b/include/block/block.h index f374e1a725..e18233afe0 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -107,6 +107,7 @@ typedef struct HDGeometry { #define BDRV_OPT_CACHE_WB "cache.writeback" #define BDRV_OPT_CACHE_DIRECT "cache.direct" #define BDRV_OPT_CACHE_NO_FLUSH "cache.no-flush" +#define BDRV_OPT_READ_ONLY "read-only" #define BDRV_SECTOR_BITS 9 From 5b7ba05fe7313b03712e129a86fa70c2c215e908 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Thu, 15 Sep 2016 17:53:03 +0300 Subject: [PATCH 08/33] block: Don't queue the same BDS twice in bdrv_reopen_queue_child() bdrv_reopen_queue_child() assumes that a BlockDriverState is never added twice to BlockReopenQueue. That's however not the case: commit_start() adds 'base' (and its children) to a new reopen queue, and then 'overlay_bs' (and its children, which include 'base') to the same queue. The effect of this is that the first set of options is ignored and overriden by the second. We fixed this by swapping the order in which both BDSs were added to the queue in 3db2bd5508c86a1605258bc77c9672d93b5c350e. This patch checks if a BDS is already in the reopen queue and keeps its options. Signed-off-by: Alberto Garcia Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index b724adacfc..493ecf3137 100644 --- a/block.c +++ b/block.c @@ -1925,6 +1925,13 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, options = qdict_new(); } + /* Check if this BlockDriverState is already in the queue */ + QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) { + if (bs == bs_entry->state.bs) { + break; + } + } + /* * Precedence of options: * 1. Explicitly passed in options (highest) @@ -1945,7 +1952,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, } /* Old explicitly set values (don't overwrite by inherited value) */ - old_options = qdict_clone_shallow(bs->explicit_options); + if (bs_entry) { + old_options = qdict_clone_shallow(bs_entry->state.explicit_options); + } else { + old_options = qdict_clone_shallow(bs->explicit_options); + } bdrv_join_options(bs, options, old_options); QDECREF(old_options); @@ -1984,8 +1995,13 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, child->role, options, flags); } - bs_entry = g_new0(BlockReopenQueueEntry, 1); - QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry); + if (!bs_entry) { + bs_entry = g_new0(BlockReopenQueueEntry, 1); + QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry); + } else { + QDECREF(bs_entry->state.options); + QDECREF(bs_entry->state.explicit_options); + } bs_entry->state.bs = bs; bs_entry->state.options = options; From 0fe282bb4b29ad51adefc2e500bcecfb3c499e10 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Thu, 15 Sep 2016 17:53:04 +0300 Subject: [PATCH 09/33] commit: Add 'base' to the reopen queue before 'overlay_bs' Now that we're checking for duplicates in the reopen queue, there's no need to force a specific order in which the queue is constructed so we can revert 3db2bd5508c86a1605258bc77c9672d93b5c350e. Since both ways of constructing the queue are now valid, this patch doesn't have any effect on the behavior of QEMU and is not strictly necessary. However it can help us check that the fix for the reopen queue is robust: if it stops working properly at some point, iotest 040 will break. Signed-off-by: Alberto Garcia Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/commit.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/block/commit.c b/block/commit.c index a02539bacc..9f67a8b121 100644 --- a/block/commit.c +++ b/block/commit.c @@ -242,14 +242,14 @@ void commit_start(const char *job_id, BlockDriverState *bs, orig_overlay_flags = bdrv_get_flags(overlay_bs); /* convert base & overlay_bs to r/w, if necessary */ - if (!(orig_overlay_flags & BDRV_O_RDWR)) { - reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL, - orig_overlay_flags | BDRV_O_RDWR); - } if (!(orig_base_flags & BDRV_O_RDWR)) { reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL, orig_base_flags | BDRV_O_RDWR); } + if (!(orig_overlay_flags & BDRV_O_RDWR)) { + reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL, + orig_overlay_flags | BDRV_O_RDWR); + } if (reopen_queue) { bdrv_reopen_multiple(reopen_queue, &local_err); if (local_err != NULL) { From 4e200cf8e64cc37abf00c53b299a794c80276360 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Thu, 15 Sep 2016 17:53:05 +0300 Subject: [PATCH 10/33] block: rename "read-only" to BDRV_OPT_READ_ONLY There were a few instances left. After this patch we're using the macro in all places. Signed-off-by: Alberto Garcia Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- blockdev.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/blockdev.c b/blockdev.c index c3f8da4700..fb207cd4c1 100644 --- a/blockdev.c +++ b/blockdev.c @@ -807,7 +807,7 @@ QemuOptsList qemu_legacy_drive_opts = { /* Options that are passed on, but have special semantics with -drive */ { - .name = "read-only", + .name = BDRV_OPT_READ_ONLY, .type = QEMU_OPT_BOOL, .help = "open drive file as read-only", },{ @@ -873,7 +873,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) { "group", "throttling.group" }, - { "readonly", "read-only" }, + { "readonly", BDRV_OPT_READ_ONLY }, }; for (i = 0; i < ARRAY_SIZE(opt_renames); i++) { @@ -945,7 +945,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) } /* copy-on-read is disabled with a warning for read-only devices */ - read_only |= qemu_opt_get_bool(legacy_opts, "read-only", false); + read_only |= qemu_opt_get_bool(legacy_opts, BDRV_OPT_READ_ONLY, false); copy_on_read = qemu_opt_get_bool(legacy_opts, "copy-on-read", false); if (read_only && copy_on_read) { @@ -953,7 +953,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) copy_on_read = false; } - qdict_put(bs_opts, "read-only", + qdict_put(bs_opts, BDRV_OPT_READ_ONLY, qstring_from_str(read_only ? "on" : "off")); qdict_put(bs_opts, "copy-on-read", qstring_from_str(copy_on_read ? "on" :"off")); @@ -4042,7 +4042,7 @@ QemuOptsList qemu_common_drive_opts = { .type = QEMU_OPT_STRING, .help = "write error action", },{ - .name = "read-only", + .name = BDRV_OPT_READ_ONLY, .type = QEMU_OPT_BOOL, .help = "open drive file as read-only", },{ From 6bed02805666035a59daa6a46935074f6f6d507d Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 16 Sep 2016 17:42:25 +0200 Subject: [PATCH 11/33] block: Fix 'since' for compressed Drive/BlockdevBackup These patches missed 2.7, update the QAPI documentation. Reported-by: Eric Blake Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- qapi/block-core.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 24223fd08a..eb0a7d5aab 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -900,7 +900,7 @@ # otherwise. (Since 2.4) # # @compress: #optional true to compress data, if the target format supports it. -# (default: false) (since 2.7) +# (default: false) (since 2.8) # # @on-source-error: #optional the action to take on an error on the source, # default 'report'. 'stop' and 'enospc' can only be used @@ -941,7 +941,7 @@ # for unlimited. # # @compress: #optional true to compress data, if the target format supports it. -# (default: false) (since 2.7) +# (default: false) (since 2.8) # # @on-source-error: #optional the action to take on an error on the source, # default 'report'. 'stop' and 'enospc' can only be used From 1c89e1fa2fb1414f4485a01924032ad9cdb9073d Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 20 Sep 2016 13:38:40 +0200 Subject: [PATCH 12/33] block: Add blk_by_dev() This finds a BlockBackend given the device model that is attached to it. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- block/block-backend.c | 19 +++++++++++++++++++ include/sysemu/block-backend.h | 1 + 2 files changed, 20 insertions(+) diff --git a/block/block-backend.c b/block/block-backend.c index d1349d90e5..0bd19abdfb 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -559,6 +559,25 @@ void *blk_get_attached_dev(BlockBackend *blk) return blk->dev; } +/* + * Return the BlockBackend which has the device model @dev attached if it + * exists, else null. + * + * @dev must not be null. + */ +BlockBackend *blk_by_dev(void *dev) +{ + BlockBackend *blk = NULL; + + assert(dev != NULL); + while ((blk = blk_all_next(blk)) != NULL) { + if (blk->dev == dev) { + return blk; + } + } + return NULL; +} + /* * Set @blk's device model callbacks to @ops. * @opaque is the opaque argument to pass to the callbacks. diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index 4808a9621a..410eb68145 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -111,6 +111,7 @@ int blk_attach_dev(BlockBackend *blk, void *dev); void blk_attach_dev_nofail(BlockBackend *blk, void *dev); void blk_detach_dev(BlockBackend *blk, void *dev); void *blk_get_attached_dev(BlockBackend *blk); +BlockBackend *blk_by_dev(void *dev); void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque); int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf, int count); From 6c1db528b0e1a68c342d49d032af1c707e9de87c Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 20 Sep 2016 13:38:41 +0200 Subject: [PATCH 13/33] qdev-monitor: Factor out find_device_state() Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- qdev-monitor.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/qdev-monitor.c b/qdev-monitor.c index e19617fa8b..bc0213f5e3 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -801,7 +801,7 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) object_unref(OBJECT(dev)); } -void qmp_device_del(const char *id, Error **errp) +static DeviceState *find_device_state(const char *id, Error **errp) { Object *obj; @@ -819,15 +819,23 @@ void qmp_device_del(const char *id, Error **errp) if (!obj) { error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", id); - return; + return NULL; } if (!object_dynamic_cast(obj, TYPE_DEVICE)) { error_setg(errp, "%s is not a hotpluggable device", id); - return; + return NULL; } - qdev_unplug(DEVICE(obj), errp); + return DEVICE(obj); +} + +void qmp_device_del(const char *id, Error **errp) +{ + DeviceState *dev = find_device_state(id, errp); + if (dev != NULL) { + qdev_unplug(dev, errp); + } } void qdev_machine_init(void) From 9680caee0fa530726809bc72d44e127ae676e251 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 20 Sep 2016 13:38:42 +0200 Subject: [PATCH 14/33] qdev-monitor: Add blk_by_qdev_id() This finds the BlockBackend attached to the device model identified by its qdev ID. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- include/sysemu/block-backend.h | 1 + qdev-monitor.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index 410eb68145..3b29317349 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -112,6 +112,7 @@ void blk_attach_dev_nofail(BlockBackend *blk, void *dev); void blk_detach_dev(BlockBackend *blk, void *dev); void *blk_get_attached_dev(BlockBackend *blk); BlockBackend *blk_by_dev(void *dev); +BlockBackend *blk_by_qdev_id(const char *id, Error **errp); void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque); int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf, int count); diff --git a/qdev-monitor.c b/qdev-monitor.c index bc0213f5e3..4f78ecb091 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -28,6 +28,7 @@ #include "qemu/config-file.h" #include "qemu/error-report.h" #include "qemu/help_option.h" +#include "sysemu/block-backend.h" /* * Aliases were a bad idea from the start. Let's keep them @@ -838,6 +839,23 @@ void qmp_device_del(const char *id, Error **errp) } } +BlockBackend *blk_by_qdev_id(const char *id, Error **errp) +{ + DeviceState *dev; + BlockBackend *blk; + + dev = find_device_state(id, errp); + if (dev == NULL) { + return NULL; + } + + blk = blk_by_dev(dev); + if (!blk) { + error_setg(errp, "Device does not have a block device backend"); + } + return blk; +} + void qdev_machine_init(void) { qdev_get_peripheral_anon(); From b33945cfffcc3f847122dbf5db00fff28161c593 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 20 Sep 2016 13:38:43 +0200 Subject: [PATCH 15/33] block: Accept device model name for blockdev-open/close-tray In order to remove the need for BlockBackend names in the external API, we want to allow qdev device names in all device related commands. This converts blockdev-open/close-tray to accept a qdev device name. Signed-off-by: Kevin Wolf --- blockdev.c | 61 ++++++++++++++++++++++++++++++++----------- docs/qmp-commands.txt | 12 ++++++--- qapi/block-core.json | 14 +++++++--- 3 files changed, 64 insertions(+), 23 deletions(-) diff --git a/blockdev.c b/blockdev.c index fb207cd4c1..046f9c6cd4 100644 --- a/blockdev.c +++ b/blockdev.c @@ -56,7 +56,8 @@ static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states = QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states); -static int do_open_tray(const char *device, bool force, Error **errp); +static int do_open_tray(const char *blk_name, const char *qdev_id, + bool force, Error **errp); static const char *const if_name[IF_COUNT] = { [IF_NONE] = "none", @@ -1198,6 +1199,29 @@ static BlockDriverState *qmp_get_root_bs(const char *name, Error **errp) return bs; } +static BlockBackend *qmp_get_blk(const char *blk_name, const char *qdev_id, + Error **errp) +{ + BlockBackend *blk; + + if (!blk_name == !qdev_id) { + error_setg(errp, "Need exactly one of 'device' and 'id'"); + return NULL; + } + + if (qdev_id) { + blk = blk_by_qdev_id(qdev_id, errp); + } else { + blk = blk_by_name(blk_name); + if (blk == NULL) { + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", blk_name); + } + } + + return blk; +} + void hmp_commit(Monitor *mon, const QDict *qdict) { const char *device = qdict_get_str(qdict, "device"); @@ -2250,7 +2274,7 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp) force = false; } - rc = do_open_tray(device, force, &local_err); + rc = do_open_tray(device, NULL, force, &local_err); if (rc && rc != -ENOSYS) { error_propagate(errp, local_err); return; @@ -2295,15 +2319,15 @@ void qmp_block_passwd(bool has_device, const char *device, * If the guest was asked to open the tray, return -EINPROGRESS. * Else, return 0. */ -static int do_open_tray(const char *device, bool force, Error **errp) +static int do_open_tray(const char *blk_name, const char *qdev_id, + bool force, Error **errp) { BlockBackend *blk; + const char *device = qdev_id ?: blk_name; bool locked; - blk = blk_by_name(device); + blk = qmp_get_blk(blk_name, qdev_id, errp); if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); return -ENODEV; } @@ -2339,7 +2363,9 @@ static int do_open_tray(const char *device, bool force, Error **errp) return 0; } -void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, +void qmp_blockdev_open_tray(bool has_device, const char *device, + bool has_id, const char *id, + bool has_force, bool force, Error **errp) { Error *local_err = NULL; @@ -2348,7 +2374,9 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, if (!has_force) { force = false; } - rc = do_open_tray(device, force, &local_err); + rc = do_open_tray(has_device ? device : NULL, + has_id ? id : NULL, + force, &local_err); if (rc && rc != -ENOSYS && rc != -EINPROGRESS) { error_propagate(errp, local_err); return; @@ -2356,19 +2384,22 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, error_free(local_err); } -void qmp_blockdev_close_tray(const char *device, Error **errp) +void qmp_blockdev_close_tray(bool has_device, const char *device, + bool has_id, const char *id, + Error **errp) { BlockBackend *blk; - blk = blk_by_name(device); + device = has_device ? device : NULL; + id = has_id ? id : NULL; + + blk = qmp_get_blk(device, id, errp); if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); return; } if (!blk_dev_has_removable_media(blk)) { - error_setg(errp, "Device '%s' is not removable", device); + error_setg(errp, "Device '%s' is not removable", device ?: id); return; } @@ -2564,7 +2595,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename, goto fail; } - rc = do_open_tray(device, false, &err); + rc = do_open_tray(device, NULL, false, &err); if (rc && rc != -ENOSYS) { error_propagate(errp, err); goto fail; @@ -2586,7 +2617,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename, blk_apply_root_state(blk, medium_bs); - qmp_blockdev_close_tray(device, errp); + qmp_blockdev_close_tray(true, device, false, NULL, errp); fail: /* If the medium has been inserted, the device has its own reference, so diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt index acebeb3954..9e230f51f7 100644 --- a/docs/qmp-commands.txt +++ b/docs/qmp-commands.txt @@ -3228,7 +3228,9 @@ which no such event will be generated, these include: Arguments: -- "device": block device name (json-string) +- "device": block device name (deprecated, use @id instead) + (json-string, optional) +- "id": the name or QOM path of the guest device (json-string, optional) - "force": if false (the default), an eject request will be sent to the guest if it has locked the tray (and the tray will not be opened immediately); if true, the tray will be opened regardless of whether it is locked @@ -3237,7 +3239,7 @@ Arguments: Example: -> { "execute": "blockdev-open-tray", - "arguments": { "device": "ide1-cd0" } } + "arguments": { "id": "ide0-1-0" } } <- { "timestamp": { "seconds": 1418751016, "microseconds": 716996 }, @@ -3258,12 +3260,14 @@ If the tray was already closed before, this will be a no-op. Arguments: -- "device": block device name (json-string) +- "device": block device name (deprecated, use @id instead) + (json-string, optional) +- "id": the name or QOM path of the guest device (json-string, optional) Example: -> { "execute": "blockdev-close-tray", - "arguments": { "device": "ide1-cd0" } } + "arguments": { "id": "ide0-1-0" } } <- { "timestamp": { "seconds": 1418751345, "microseconds": 272147 }, diff --git a/qapi/block-core.json b/qapi/block-core.json index eb0a7d5aab..cd7b38aa53 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2363,7 +2363,9 @@ # to it # - if the guest device does not have an actual tray # -# @device: block device name +# @device: #optional Block device name (deprecated, use @id instead) +# +# @id: #optional The name or QOM path of the guest device (since: 2.8) # # @force: #optional if false (the default), an eject request will be sent to # the guest if it has locked the tray (and the tray will not be opened @@ -2373,7 +2375,8 @@ # Since: 2.5 ## { 'command': 'blockdev-open-tray', - 'data': { 'device': 'str', + 'data': { '*device': 'str', + '*id': 'str', '*force': 'bool' } } ## @@ -2385,12 +2388,15 @@ # # If the tray was already closed before, this will be a no-op. # -# @device: block device name +# @device: #optional Block device name (deprecated, use @id instead) +# +# @id: #optional The name or QOM path of the guest device (since: 2.8) # # Since: 2.5 ## { 'command': 'blockdev-close-tray', - 'data': { 'device': 'str' } } + 'data': { '*device': 'str', + '*id': 'str' } } ## # @x-blockdev-remove-medium: From 716df21707b9c95d61c86e1df9105d0cefe59a97 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 20 Sep 2016 13:38:44 +0200 Subject: [PATCH 16/33] block: Accept device model name for x-blockdev-insert-medium In order to remove the need for BlockBackend names in the external API, we want to allow qdev device names in all device related commands. This converts x-blockdev-insert-medium to accept a qdev device name. As the command is experimental, we can still remove the 'device' option that uses the BlockBackend name. This requires some test case changes and is left for another series. Signed-off-by: Kevin Wolf --- blockdev.c | 33 +++++++++++++++++---------------- docs/qmp-commands.txt | 6 ++++-- qapi/block-core.json | 7 +++++-- 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/blockdev.c b/blockdev.c index 046f9c6cd4..0eb173d819 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2468,34 +2468,26 @@ out: aio_context_release(aio_context); } -static void qmp_blockdev_insert_anon_medium(const char *device, +static void qmp_blockdev_insert_anon_medium(BlockBackend *blk, BlockDriverState *bs, Error **errp) { - BlockBackend *blk; bool has_device; - blk = blk_by_name(device); - if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); - return; - } - /* For BBs without a device, we can exchange the BDS tree at will */ has_device = blk_get_attached_dev(blk); if (has_device && !blk_dev_has_removable_media(blk)) { - error_setg(errp, "Device '%s' is not removable", device); + error_setg(errp, "Device is not removable"); return; } if (has_device && blk_dev_has_tray(blk) && !blk_dev_is_tray_open(blk)) { - error_setg(errp, "Tray of device '%s' is not open", device); + error_setg(errp, "Tray of the device is not open"); return; } if (blk_bs(blk)) { - error_setg(errp, "There already is a medium in device '%s'", device); + error_setg(errp, "There already is a medium in the device"); return; } @@ -2511,11 +2503,20 @@ static void qmp_blockdev_insert_anon_medium(const char *device, } } -void qmp_x_blockdev_insert_medium(const char *device, const char *node_name, - Error **errp) +void qmp_x_blockdev_insert_medium(bool has_device, const char *device, + bool has_id, const char *id, + const char *node_name, Error **errp) { + BlockBackend *blk; BlockDriverState *bs; + blk = qmp_get_blk(has_device ? device : NULL, + has_id ? id : NULL, + errp); + if (!blk) { + return; + } + bs = bdrv_find_node(node_name); if (!bs) { error_setg(errp, "Node '%s' not found", node_name); @@ -2528,7 +2529,7 @@ void qmp_x_blockdev_insert_medium(const char *device, const char *node_name, return; } - qmp_blockdev_insert_anon_medium(device, bs, errp); + qmp_blockdev_insert_anon_medium(blk, bs, errp); } void qmp_blockdev_change_medium(const char *device, const char *filename, @@ -2609,7 +2610,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename, goto fail; } - qmp_blockdev_insert_anon_medium(device, medium_bs, &err); + qmp_blockdev_insert_anon_medium(blk, medium_bs, &err); if (err) { error_propagate(errp, err); goto fail; diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt index 9e230f51f7..ebb65e0ad8 100644 --- a/docs/qmp-commands.txt +++ b/docs/qmp-commands.txt @@ -3328,7 +3328,9 @@ Stay away from it unless you want to help with its development. Arguments: -- "device": block device name (json-string) +- "device": block device name (deprecated, use @id instead) + (json-string, optional) +- "id": the name or QOM path of the guest device (json-string, optional) - "node-name": root node of the BDS tree to insert into the block device Example: @@ -3342,7 +3344,7 @@ Example: <- { "return": {} } -> { "execute": "x-blockdev-insert-medium", - "arguments": { "device": "ide1-cd0", + "arguments": { "id": "ide0-1-0", "node-name": "node0" } } <- { "return": {} } diff --git a/qapi/block-core.json b/qapi/block-core.json index cd7b38aa53..6ac680966f 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2427,14 +2427,17 @@ # This command is still a work in progress and is considered experimental. # Stay away from it unless you want to help with its development. # -# @device: block device name +# @device: #optional Block device name (deprecated, use @id instead) +# +# @id: #optional The name or QOM path of the guest device (since: 2.8) # # @node-name: name of a node in the block driver state graph # # Since: 2.5 ## { 'command': 'x-blockdev-insert-medium', - 'data': { 'device': 'str', + 'data': { '*device': 'str', + '*id': 'str', 'node-name': 'str'} } From 00949babe90c528df1ffb79439d632c7bd4f6c42 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 20 Sep 2016 13:38:45 +0200 Subject: [PATCH 17/33] block: Accept device model name for x-blockdev-remove-medium In order to remove the need for BlockBackend names in the external API, we want to allow qdev device names in all device related commands. This converts x-blockdev-remove-medium to accept a qdev device name. As the command is experimental, we can still remove the 'device' option that uses the BlockBackend name. This requires some test case changes and is left for another series. Signed-off-by: Kevin Wolf --- blockdev.c | 28 ++++++++++++++++------------ docs/qmp-commands.txt | 12 +++++++----- qapi/block-core.json | 7 +++++-- 3 files changed, 28 insertions(+), 19 deletions(-) diff --git a/blockdev.c b/blockdev.c index 0eb173d819..a007b22f80 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2281,7 +2281,7 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp) } error_free(local_err); - qmp_x_blockdev_remove_medium(device, errp); + qmp_x_blockdev_remove_medium(true, device, false, NULL, errp); } void qmp_block_passwd(bool has_device, const char *device, @@ -2415,30 +2415,34 @@ void qmp_blockdev_close_tray(bool has_device, const char *device, blk_dev_change_media_cb(blk, true); } -void qmp_x_blockdev_remove_medium(const char *device, Error **errp) +void qmp_x_blockdev_remove_medium(bool has_device, const char *device, + bool has_id, const char *id, Error **errp) { BlockBackend *blk; BlockDriverState *bs; AioContext *aio_context; - bool has_device; + bool has_attached_device; - blk = blk_by_name(device); + device = has_device ? device : NULL; + id = has_id ? id : NULL; + + blk = qmp_get_blk(device, id, errp); if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); return; } /* For BBs without a device, we can exchange the BDS tree at will */ - has_device = blk_get_attached_dev(blk); + has_attached_device = blk_get_attached_dev(blk); - if (has_device && !blk_dev_has_removable_media(blk)) { - error_setg(errp, "Device '%s' is not removable", device); + if (has_attached_device && !blk_dev_has_removable_media(blk)) { + error_setg(errp, "Device '%s' is not removable", device ?: id); return; } - if (has_device && blk_dev_has_tray(blk) && !blk_dev_is_tray_open(blk)) { - error_setg(errp, "Tray of device '%s' is not open", device); + if (has_attached_device && blk_dev_has_tray(blk) && + !blk_dev_is_tray_open(blk)) + { + error_setg(errp, "Tray of device '%s' is not open", device ?: id); return; } @@ -2604,7 +2608,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename, error_free(err); err = NULL; - qmp_x_blockdev_remove_medium(device, &err); + qmp_x_blockdev_remove_medium(true, device, false, NULL, errp); if (err) { error_propagate(errp, err); goto fail; diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt index ebb65e0ad8..e77bf2f1dd 100644 --- a/docs/qmp-commands.txt +++ b/docs/qmp-commands.txt @@ -3290,18 +3290,20 @@ Stay away from it unless you want to help with its development. Arguments: -- "device": block device name (json-string) +- "device": block device name (deprecated, use @id instead) + (json-string, optional) +- "id": the name or QOM path of the guest device (json-string, optional) Example: -> { "execute": "x-blockdev-remove-medium", - "arguments": { "device": "ide1-cd0" } } + "arguments": { "id": "ide0-1-0" } } <- { "error": { "class": "GenericError", - "desc": "Tray of device 'ide1-cd0' is not open" } } + "desc": "Tray of device 'ide0-1-0' is not open" } } -> { "execute": "blockdev-open-tray", - "arguments": { "device": "ide1-cd0" } } + "arguments": { "id": "ide0-1-0" } } <- { "timestamp": { "seconds": 1418751627, "microseconds": 549958 }, @@ -3312,7 +3314,7 @@ Example: <- { "return": {} } -> { "execute": "x-blockdev-remove-medium", - "arguments": { "device": "ide1-cd0" } } + "arguments": { "device": "ide0-1-0" } } <- { "return": {} } diff --git a/qapi/block-core.json b/qapi/block-core.json index 6ac680966f..e370366786 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2410,12 +2410,15 @@ # This command is still a work in progress and is considered experimental. # Stay away from it unless you want to help with its development. # -# @device: block device name +# @device: #optional Block device name (deprecated, use @id instead) +# +# @id: #optional The name or QOM path of the guest device (since: 2.8) # # Since: 2.5 ## { 'command': 'x-blockdev-remove-medium', - 'data': { 'device': 'str' } } + 'data': { '*device': 'str', + '*id': 'str' } } ## # @x-blockdev-insert-medium: From fbe2d8163e8900fe22c67f55bd09ebc6f322f430 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 20 Sep 2016 13:38:46 +0200 Subject: [PATCH 18/33] block: Accept device model name for eject In order to remove the need for BlockBackend names in the external API, we want to allow qdev device names in all device related commands. This converts eject to accept a qdev device name. Signed-off-by: Kevin Wolf --- blockdev.c | 10 +++++++--- docs/qmp-commands.txt | 8 +++++--- hmp.c | 2 +- qapi/block.json | 9 +++++++-- ui/cocoa.m | 3 ++- 5 files changed, 22 insertions(+), 10 deletions(-) diff --git a/blockdev.c b/blockdev.c index a007b22f80..3d92724d0f 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2265,7 +2265,9 @@ exit: block_job_txn_unref(block_job_txn); } -void qmp_eject(const char *device, bool has_force, bool force, Error **errp) +void qmp_eject(bool has_device, const char *device, + bool has_id, const char *id, + bool has_force, bool force, Error **errp) { Error *local_err = NULL; int rc; @@ -2274,14 +2276,16 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp) force = false; } - rc = do_open_tray(device, NULL, force, &local_err); + rc = do_open_tray(has_device ? device : NULL, + has_id ? id : NULL, + force, &local_err); if (rc && rc != -ENOSYS) { error_propagate(errp, local_err); return; } error_free(local_err); - qmp_x_blockdev_remove_medium(true, device, false, NULL, errp); + qmp_x_blockdev_remove_medium(has_device, device, has_id, id, errp); } void qmp_block_passwd(bool has_device, const char *device, diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt index e77bf2f1dd..9024ef2823 100644 --- a/docs/qmp-commands.txt +++ b/docs/qmp-commands.txt @@ -72,12 +72,14 @@ Eject a removable medium. Arguments: -- force: force ejection (json-bool, optional) -- device: device name (json-string) +- "force": force ejection (json-bool, optional) +- "device": block device name (deprecated, use @id instead) + (json-string, optional) +- "id": the name or QOM path of the guest device (json-string, optional) Example: --> { "execute": "eject", "arguments": { "device": "ide1-cd0" } } +-> { "execute": "eject", "arguments": { "id": "ide0-1-0" } } <- { "return": {} } Note: The "force" argument defaults to false. diff --git a/hmp.c b/hmp.c index 0a16aefc2a..09827b3ec4 100644 --- a/hmp.c +++ b/hmp.c @@ -1376,7 +1376,7 @@ void hmp_eject(Monitor *mon, const QDict *qdict) const char *device = qdict_get_str(qdict, "device"); Error *err = NULL; - qmp_eject(device, true, force, &err); + qmp_eject(true, device, false, NULL, true, force, &err); hmp_handle_error(mon, &err); } diff --git a/qapi/block.json b/qapi/block.json index 8b08bd2fd0..c896bd1d3b 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -125,7 +125,9 @@ # # Ejects a device from a removable drive. # -# @device: The name of the device +# @device: #optional Block device name (deprecated, use @id instead) +# +# @id: #optional The name or QOM path of the guest device (since: 2.8) # # @force: @optional If true, eject regardless of whether the drive is locked. # If not specified, the default value is false. @@ -137,7 +139,10 @@ # # Since: 0.14.0 ## -{ 'command': 'eject', 'data': {'device': 'str', '*force': 'bool'} } +{ 'command': 'eject', + 'data': { '*device': 'str', + '*id': 'str', + '*force': 'bool' } } ## # @nbd-server-start: diff --git a/ui/cocoa.m b/ui/cocoa.m index 52d9c54b6d..a847737b68 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -1085,7 +1085,8 @@ QemuCocoaView *cocoaView; } Error *err = NULL; - qmp_eject([drive cStringUsingEncoding: NSASCIIStringEncoding], false, false, &err); + qmp_eject(true, [drive cStringUsingEncoding: NSASCIIStringEncoding], + false, NULL, false, false, &err); handleAnyDeviceErrors(err); } From 70e2cb3bd75fc7aa988f81eae854001e8fcbffe1 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 20 Sep 2016 13:38:47 +0200 Subject: [PATCH 19/33] block: Accept device model name for blockdev-change-medium In order to remove the need for BlockBackend names in the external API, we want to allow qdev device names in all device related commands. This converts blockdev-change-medium to accept a qdev device name. Signed-off-by: Kevin Wolf --- blockdev.c | 18 +++++++++++------- docs/qmp-commands.txt | 10 ++++++---- hmp.c | 5 +++-- qapi/block-core.json | 8 ++++++-- qmp.c | 4 ++-- ui/cocoa.m | 4 +++- 6 files changed, 31 insertions(+), 18 deletions(-) diff --git a/blockdev.c b/blockdev.c index 3d92724d0f..8c8fcd6750 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2540,7 +2540,9 @@ void qmp_x_blockdev_insert_medium(bool has_device, const char *device, qmp_blockdev_insert_anon_medium(blk, bs, errp); } -void qmp_blockdev_change_medium(const char *device, const char *filename, +void qmp_blockdev_change_medium(bool has_device, const char *device, + bool has_id, const char *id, + const char *filename, bool has_format, const char *format, bool has_read_only, BlockdevChangeReadOnlyMode read_only, @@ -2553,10 +2555,10 @@ void qmp_blockdev_change_medium(const char *device, const char *filename, QDict *options = NULL; Error *err = NULL; - blk = blk_by_name(device); + blk = qmp_get_blk(has_device ? device : NULL, + has_id ? id : NULL, + errp); if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); goto fail; } @@ -2604,7 +2606,9 @@ void qmp_blockdev_change_medium(const char *device, const char *filename, goto fail; } - rc = do_open_tray(device, NULL, false, &err); + rc = do_open_tray(has_device ? device : NULL, + has_id ? id : NULL, + false, &err); if (rc && rc != -ENOSYS) { error_propagate(errp, err); goto fail; @@ -2612,7 +2616,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename, error_free(err); err = NULL; - qmp_x_blockdev_remove_medium(true, device, false, NULL, errp); + qmp_x_blockdev_remove_medium(has_device, device, has_id, id, errp); if (err) { error_propagate(errp, err); goto fail; @@ -2626,7 +2630,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename, blk_apply_root_state(blk, medium_bs); - qmp_blockdev_close_tray(true, device, false, NULL, errp); + qmp_blockdev_close_tray(has_device, device, has_id, id, errp); fail: /* If the medium has been inserted, the device has its own reference, so diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt index 9024ef2823..8b2a0ea176 100644 --- a/docs/qmp-commands.txt +++ b/docs/qmp-commands.txt @@ -3458,7 +3458,9 @@ and loading a new image file which is inserted as the new medium. Arguments: -- "device": device name (json-string) +- "device": block device name (deprecated, use @id instead) + (json-string, optional) +- "id": the name or QOM path of the guest device (json-string, optional) - "filename": filename of the new image (json-string) - "format": format of the new image (json-string, optional) - "read-only-mode": new read-only mode (json-string, optional) @@ -3469,7 +3471,7 @@ Examples: 1. Change a removable medium -> { "execute": "blockdev-change-medium", - "arguments": { "device": "ide1-cd0", + "arguments": { "id": "ide0-1-0", "filename": "/srv/images/Fedora-12-x86_64-DVD.iso", "format": "raw" } } <- { "return": {} } @@ -3477,7 +3479,7 @@ Examples: 2. Load a read-only medium into a writable drive -> { "execute": "blockdev-change-medium", - "arguments": { "device": "isa-fd0", + "arguments": { "id": "floppyA", "filename": "/srv/images/ro.img", "format": "raw", "read-only-mode": "retain" } } @@ -3487,7 +3489,7 @@ Examples: "desc": "Could not open '/srv/images/ro.img': Permission denied" } } -> { "execute": "blockdev-change-medium", - "arguments": { "device": "isa-fd0", + "arguments": { "id": "floppyA", "filename": "/srv/images/ro.img", "format": "raw", "read-only-mode": "read-only" } } diff --git a/hmp.c b/hmp.c index 09827b3ec4..336e7bf076 100644 --- a/hmp.c +++ b/hmp.c @@ -1422,8 +1422,9 @@ void hmp_change(Monitor *mon, const QDict *qdict) } } - qmp_blockdev_change_medium(device, target, !!arg, arg, - !!read_only, read_only_mode, &err); + qmp_blockdev_change_medium(true, device, false, NULL, target, + !!arg, arg, !!read_only, read_only_mode, + &err); if (err && error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) { error_free(err); diff --git a/qapi/block-core.json b/qapi/block-core.json index e370366786..1d7d4cc8a0 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2470,7 +2470,10 @@ # combines blockdev-open-tray, x-blockdev-remove-medium, # x-blockdev-insert-medium and blockdev-close-tray). # -# @device: block device name +# @device: #optional Block device name (deprecated, use @id instead) +# +# @id: #optional The name or QOM path of the guest device +# (since: 2.8) # # @filename: filename of the new image to be loaded # @@ -2483,7 +2486,8 @@ # Since: 2.5 ## { 'command': 'blockdev-change-medium', - 'data': { 'device': 'str', + 'data': { '*device': 'str', + '*id': 'str', 'filename': 'str', '*format': 'str', '*read-only-mode': 'BlockdevChangeReadOnlyMode' } } diff --git a/qmp.c b/qmp.c index 6733463fa2..8b4bc980f1 100644 --- a/qmp.c +++ b/qmp.c @@ -436,8 +436,8 @@ void qmp_change(const char *device, const char *target, if (strcmp(device, "vnc") == 0) { qmp_change_vnc(target, has_arg, arg, errp); } else { - qmp_blockdev_change_medium(device, target, has_arg, arg, false, 0, - errp); + qmp_blockdev_change_medium(true, device, false, NULL, target, + has_arg, arg, false, 0, errp); } } diff --git a/ui/cocoa.m b/ui/cocoa.m index a847737b68..ba0e98a297 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -1119,8 +1119,10 @@ QemuCocoaView *cocoaView; } Error *err = NULL; - qmp_blockdev_change_medium([drive cStringUsingEncoding: + qmp_blockdev_change_medium(true, + [drive cStringUsingEncoding: NSASCIIStringEncoding], + false, NULL, [file cStringUsingEncoding: NSASCIIStringEncoding], true, "raw", From 7a9877a0263561f11bae116a7639eec53a625807 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 20 Sep 2016 13:38:48 +0200 Subject: [PATCH 20/33] block: Accept device model name for block_set_io_throttle In order to remove the need for BlockBackend names in the external API, we want to allow qdev device names in all device related commands. This converts block_set_io_throttle to accept a qdev device name. Signed-off-by: Kevin Wolf --- blockdev.c | 12 +++++++----- docs/qmp-commands.txt | 6 ++++-- qapi/block-core.json | 8 +++++--- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/blockdev.c b/blockdev.c index 8c8fcd6750..405145ae95 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2647,10 +2647,10 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) BlockBackend *blk; AioContext *aio_context; - blk = blk_by_name(arg->device); + blk = qmp_get_blk(arg->has_device ? arg->device : NULL, + arg->has_id ? arg->id : NULL, + errp); if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", arg->device); return; } @@ -2659,7 +2659,7 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) bs = blk_bs(blk); if (!bs) { - error_setg(errp, "Device '%s' has no medium", arg->device); + error_setg(errp, "Device has no medium"); goto out; } @@ -2723,7 +2723,9 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) * just update the throttling group. */ if (!blk_get_public(blk)->throttle_state) { blk_io_limits_enable(blk, - arg->has_group ? arg->group : arg->device); + arg->has_group ? arg->group : + arg->has_device ? arg->device : + arg->id); } else if (arg->has_group) { blk_io_limits_update_group(blk, arg->group); } diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt index 8b2a0ea176..41f56981af 100644 --- a/docs/qmp-commands.txt +++ b/docs/qmp-commands.txt @@ -1459,7 +1459,9 @@ Change I/O throttle limits for a block drive. Arguments: -- "device": device name (json-string) +- "device": block device name (deprecated, use @id instead) + (json-string, optional) +- "id": the name or QOM path of the guest device (json-string, optional) - "bps": total throughput limit in bytes per second (json-int) - "bps_rd": read throughput limit in bytes per second (json-int) - "bps_wr": write throughput limit in bytes per second (json-int) @@ -1483,7 +1485,7 @@ Arguments: Example: --> { "execute": "block_set_io_throttle", "arguments": { "device": "virtio0", +-> { "execute": "block_set_io_throttle", "arguments": { "id": "ide0-1-0", "bps": 1000000, "bps_rd": 0, "bps_wr": 0, diff --git a/qapi/block-core.json b/qapi/block-core.json index 1d7d4cc8a0..5f04dab027 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1377,7 +1377,9 @@ # # A set of parameters describing block throttling. # -# @device: The name of the device +# @device: #optional Block device name (deprecated, use @id instead) +# +# @id: #optional The name or QOM path of the guest device (since: 2.8) # # @bps: total throughput limit in bytes per second # @@ -1446,8 +1448,8 @@ # Since: 1.1 ## { 'struct': 'BlockIOThrottle', - 'data': { 'device': 'str', 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int', - 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int', + 'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int', + 'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int', '*bps_max': 'int', '*bps_rd_max': 'int', '*bps_wr_max': 'int', '*iops_max': 'int', '*iops_rd_max': 'int', '*iops_wr_max': 'int', From 486b88bdc87eb70d26428624a1669824e093c861 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 20 Sep 2016 13:38:49 +0200 Subject: [PATCH 21/33] qemu-iotests/118: Test media change with qdev name We just added the option to use qdev device names in all device related block QMP commands. This patch converts some of the test cases in 118 to use qdev device names instead of BlockBackend names to cover the new way. It converts cases for each of the media change commands, but only for CD-ROM and not everywhere, so that the old way is still tested, too. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- tests/qemu-iotests/118 | 85 ++++++++++++++++++++++++++++------- tests/qemu-iotests/iotests.py | 5 +++ 2 files changed, 73 insertions(+), 17 deletions(-) diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118 index 9e5951f645..0380069ef6 100755 --- a/tests/qemu-iotests/118 +++ b/tests/qemu-iotests/118 @@ -62,6 +62,9 @@ class ChangeBaseClass(iotests.QMPTestCase): self.fail('Timeout while waiting for the tray to close') class GeneralChangeTestsBaseClass(ChangeBaseClass): + + device_name = None + def test_change(self): result = self.vm.qmp('change', device='drive0', target=new_img, arg=iotests.imgfmt) @@ -76,9 +79,15 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) def test_blockdev_change_medium(self): - result = self.vm.qmp('blockdev-change-medium', device='drive0', - filename=new_img, - format=iotests.imgfmt) + if self.device_name is not None: + result = self.vm.qmp('blockdev-change-medium', + id=self.device_name, filename=new_img, + format=iotests.imgfmt) + else: + result = self.vm.qmp('blockdev-change-medium', + device='drive0', filename=new_img, + format=iotests.imgfmt) + self.assert_qmp(result, 'return', {}) self.wait_for_open() @@ -90,7 +99,10 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) def test_eject(self): - result = self.vm.qmp('eject', device='drive0', force=True) + if self.device_name is not None: + result = self.vm.qmp('eject', id=self.device_name, force=True) + else: + result = self.vm.qmp('eject', device='drive0', force=True) self.assert_qmp(result, 'return', {}) self.wait_for_open() @@ -101,7 +113,10 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.assert_qmp_absent(result, 'return[0]/inserted') def test_tray_eject_change(self): - result = self.vm.qmp('eject', device='drive0', force=True) + if self.device_name is not None: + result = self.vm.qmp('eject', id=self.device_name, force=True) + else: + result = self.vm.qmp('eject', device='drive0', force=True) self.assert_qmp(result, 'return', {}) self.wait_for_open() @@ -111,9 +126,12 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.assert_qmp(result, 'return[0]/tray_open', True) self.assert_qmp_absent(result, 'return[0]/inserted') - result = self.vm.qmp('blockdev-change-medium', device='drive0', - filename=new_img, - format=iotests.imgfmt) + if self.device_name is not None: + result = self.vm.qmp('blockdev-change-medium', id=self.device_name, + filename=new_img, format=iotests.imgfmt) + else: + result = self.vm.qmp('blockdev-change-medium', device='drive0', + filename=new_img, format=iotests.imgfmt) self.assert_qmp(result, 'return', {}) self.wait_for_close() @@ -124,7 +142,12 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) def test_tray_open_close(self): - result = self.vm.qmp('blockdev-open-tray', device='drive0', force=True) + if self.device_name is not None: + result = self.vm.qmp('blockdev-open-tray', + id=self.device_name, force=True) + else: + result = self.vm.qmp('blockdev-open-tray', + device='drive0', force=True) self.assert_qmp(result, 'return', {}) self.wait_for_open() @@ -137,7 +160,10 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): else: self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) - result = self.vm.qmp('blockdev-close-tray', device='drive0') + if self.device_name is not None: + result = self.vm.qmp('blockdev-close-tray', id=self.device_name) + else: + result = self.vm.qmp('blockdev-close-tray', device='drive0') self.assert_qmp(result, 'return', {}) if self.has_real_tray or not self.was_empty: @@ -162,7 +188,10 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.assert_qmp(result, 'return[0]/tray_open', True) self.assert_qmp_absent(result, 'return[0]/inserted') - result = self.vm.qmp('blockdev-close-tray', device='drive0') + if self.device_name is not None: + result = self.vm.qmp('blockdev-close-tray', id=self.device_name) + else: + result = self.vm.qmp('blockdev-close-tray', device='drive0') self.assert_qmp(result, 'return', {}) self.wait_for_close() @@ -206,7 +235,12 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): 'driver': 'file'}}) self.assert_qmp(result, 'return', {}) - result = self.vm.qmp('blockdev-open-tray', device='drive0', force=True) + if self.device_name is not None: + result = self.vm.qmp('blockdev-open-tray', + id=self.device_name, force=True) + else: + result = self.vm.qmp('blockdev-open-tray', + device='drive0', force=True) self.assert_qmp(result, 'return', {}) self.wait_for_open() @@ -219,7 +253,11 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): else: self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) - result = self.vm.qmp('x-blockdev-remove-medium', device='drive0') + if self.device_name is not None: + result = self.vm.qmp('x-blockdev-remove-medium', + id=self.device_name) + else: + result = self.vm.qmp('x-blockdev-remove-medium', device='drive0') self.assert_qmp(result, 'return', {}) result = self.vm.qmp('query-block') @@ -227,8 +265,12 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.assert_qmp(result, 'return[0]/tray_open', True) self.assert_qmp_absent(result, 'return[0]/inserted') - result = self.vm.qmp('x-blockdev-insert-medium', device='drive0', - node_name='new') + if self.device_name is not None: + result = self.vm.qmp('x-blockdev-insert-medium', + id=self.device_name, node_name='new') + else: + result = self.vm.qmp('x-blockdev-insert-medium', + device='drive0', node_name='new') self.assert_qmp(result, 'return', {}) result = self.vm.qmp('query-block') @@ -236,7 +278,10 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.assert_qmp(result, 'return[0]/tray_open', True) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) - result = self.vm.qmp('blockdev-close-tray', device='drive0') + if self.device_name is not None: + result = self.vm.qmp('blockdev-close-tray', id=self.device_name) + else: + result = self.vm.qmp('blockdev-close-tray', device='drive0') self.assert_qmp(result, 'return', {}) self.wait_for_close() @@ -280,7 +325,13 @@ class TestInitiallyFilled(GeneralChangeTestsBaseClass): def setUp(self, media, interface): qemu_img('create', '-f', iotests.imgfmt, old_img, '1440k') qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k') - self.vm = iotests.VM().add_drive(old_img, 'media=%s' % media, interface) + self.vm = iotests.VM() + if interface == 'ide': + self.device_name = 'qdev0' + self.vm.add_drive(old_img, 'media=%s' % media, 'none') + self.vm.add_device('ide-cd,drive=drive0,id=%s' % self.device_name) + else: + self.vm.add_drive(old_img, 'media=%s' % media, interface) self.vm.launch() def tearDown(self): diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index f1f36d7fc7..3329bc1721 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -139,6 +139,11 @@ class VM(qtest.QEMUQtestMachine): self._debug = True self._num_drives = 0 + def add_device(self, opts): + self._args.append('-device') + self._args.append(opts) + return self + def add_drive_raw(self, opts): self._args.append('-drive') self._args.append(opts) From 476fb028bf21cd44f5e3a72c856147a00e74f670 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 21 Sep 2016 14:56:00 +0200 Subject: [PATCH 22/33] qemu-iotests/041: Avoid blockdev-add with id We want to remove the 'id' option for blockdev-add. This removes one user of the option and makes it use only node names. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- tests/qemu-iotests/041 | 71 +++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 39 deletions(-) diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 index 80939c0d0d..d1e1ad8bd2 100755 --- a/tests/qemu-iotests/041 +++ b/tests/qemu-iotests/041 @@ -782,7 +782,7 @@ class TestRepairQuorum(iotests.QMPTestCase): self.vm.launch() #assemble the quorum block device from the individual files - args = { "options" : { "driver": "quorum", "id": "quorum0", + args = { "options" : { "driver": "quorum", "node-name": "quorum0", "vote-threshold": 2, "children": [ "img0", "img1", "img2" ] } } if self.has_quorum(): result = self.vm.qmp("blockdev-add", **args) @@ -804,13 +804,12 @@ class TestRepairQuorum(iotests.QMPTestCase): self.assert_no_active_block_jobs() - result = self.vm.qmp('drive-mirror', device='quorum0', sync='full', - node_name="repair0", - replaces="img1", + result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0', + sync='full', node_name="repair0", replaces="img1", target=quorum_repair_img, format=iotests.imgfmt) self.assert_qmp(result, 'return', {}) - self.complete_and_wait(drive="quorum0") + self.complete_and_wait(drive="job0") self.assert_has_block_node("repair0", quorum_repair_img) # TODO: a better test requiring some QEMU infrastructure will be added # to check that this file is really driven by quorum @@ -824,13 +823,12 @@ class TestRepairQuorum(iotests.QMPTestCase): self.assert_no_active_block_jobs() - result = self.vm.qmp('drive-mirror', device='quorum0', sync='full', - node_name="repair0", - replaces="img1", + result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0', + sync='full', node_name="repair0", replaces="img1", target=quorum_repair_img, format=iotests.imgfmt) self.assert_qmp(result, 'return', {}) - self.cancel_and_wait(drive="quorum0", force=True) + self.cancel_and_wait(drive="job0", force=True) # here we check that the last registered quorum file has not been # swapped out and unref self.assert_has_block_node(None, quorum_img3) @@ -842,13 +840,12 @@ class TestRepairQuorum(iotests.QMPTestCase): self.assert_no_active_block_jobs() - result = self.vm.qmp('drive-mirror', device='quorum0', sync='full', - node_name="repair0", - replaces="img1", + result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0', + sync='full', node_name="repair0", replaces="img1", target=quorum_repair_img, format=iotests.imgfmt) self.assert_qmp(result, 'return', {}) - self.wait_ready_and_cancel(drive="quorum0") + self.wait_ready_and_cancel(drive="job0") # here we check that the last registered quorum file has not been # swapped out and unref self.assert_has_block_node(None, quorum_img3) @@ -862,13 +859,12 @@ class TestRepairQuorum(iotests.QMPTestCase): self.assert_no_active_block_jobs() - result = self.vm.qmp('drive-mirror', device='quorum0', sync='full', - node_name="repair0", - replaces="img1", + result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0', + sync='full', node_name="repair0", replaces="img1", target=quorum_repair_img, format=iotests.imgfmt) self.assert_qmp(result, 'return', {}) - result = self.vm.qmp('block-job-pause', device='quorum0') + result = self.vm.qmp('block-job-pause', device='job0') self.assert_qmp(result, 'return', {}) time.sleep(1) @@ -879,10 +875,10 @@ class TestRepairQuorum(iotests.QMPTestCase): result = self.vm.qmp('query-block-jobs') self.assert_qmp(result, 'return[0]/offset', offset) - result = self.vm.qmp('block-job-resume', device='quorum0') + result = self.vm.qmp('block-job-resume', device='job0') self.assert_qmp(result, 'return', {}) - self.complete_and_wait(drive="quorum0") + self.complete_and_wait(drive="job0") self.vm.shutdown() self.assertTrue(iotests.compare_images(quorum_img2, quorum_repair_img), 'target image does not match source after mirroring') @@ -894,7 +890,7 @@ class TestRepairQuorum(iotests.QMPTestCase): if iotests.qemu_default_machine != 'pc': return - result = self.vm.qmp('drive-mirror', device='drive0', # CD-ROM + result = self.vm.qmp('drive-mirror', job_id='job0', device='drive0', # CD-ROM sync='full', node_name='repair0', replaces='img1', @@ -905,18 +901,18 @@ class TestRepairQuorum(iotests.QMPTestCase): if not self.has_quorum(): return - result = self.vm.qmp('drive-mirror', device='quorum0', sync='full', - node_name='repair0', - replaces='img1', - mode='existing', - target=quorum_repair_img, format=iotests.imgfmt) + result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0', + sync='full', node_name='repair0', replaces='img1', + mode='existing', target=quorum_repair_img, + format=iotests.imgfmt) self.assert_qmp(result, 'error/class', 'GenericError') def test_device_not_found(self): if not self.has_quorum(): return - result = self.vm.qmp('drive-mirror', device='nonexistent', sync='full', + result = self.vm.qmp('drive-mirror', job_id='job0', + device='nonexistent', sync='full', node_name='repair0', replaces='img1', target=quorum_repair_img, format=iotests.imgfmt) @@ -926,7 +922,7 @@ class TestRepairQuorum(iotests.QMPTestCase): if not self.has_quorum(): return - result = self.vm.qmp('drive-mirror', device='quorum0', + result = self.vm.qmp('drive-mirror', device='quorum0', job_id='job0', node_name='repair0', replaces='img1', target=quorum_repair_img, format=iotests.imgfmt) @@ -936,8 +932,8 @@ class TestRepairQuorum(iotests.QMPTestCase): if not self.has_quorum(): return - result = self.vm.qmp('drive-mirror', device='quorum0', sync='full', - replaces='img1', + result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0', + sync='full', replaces='img1', target=quorum_repair_img, format=iotests.imgfmt) self.assert_qmp(result, 'error/class', 'GenericError') @@ -945,9 +941,8 @@ class TestRepairQuorum(iotests.QMPTestCase): if not self.has_quorum(): return - result = self.vm.qmp('drive-mirror', device='quorum0', sync='full', - node_name='repair0', - replaces='img77', + result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0', + sync='full', node_name='repair0', replaces='img77', target=quorum_repair_img, format=iotests.imgfmt) self.assert_qmp(result, 'error/class', 'GenericError') @@ -959,19 +954,17 @@ class TestRepairQuorum(iotests.QMPTestCase): snapshot_file=quorum_snapshot_file, snapshot_node_name="snap1"); - result = self.vm.qmp('drive-mirror', device='quorum0', sync='full', - node_name='repair0', - replaces="img1", + result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0', + sync='full', node_name='repair0', replaces="img1", target=quorum_repair_img, format=iotests.imgfmt) self.assert_qmp(result, 'error/class', 'GenericError') - result = self.vm.qmp('drive-mirror', device='quorum0', sync='full', - node_name='repair0', - replaces="snap1", + result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0', + sync='full', node_name='repair0', replaces="snap1", target=quorum_repair_img, format=iotests.imgfmt) self.assert_qmp(result, 'return', {}) - self.complete_and_wait(drive="quorum0") + self.complete_and_wait('job0') self.assert_has_block_node("repair0", quorum_repair_img) # TODO: a better test requiring some QEMU infrastructure will be added # to check that this file is really driven by quorum From 522ce4ecd4e93f77ba4cd399b25db1fd5405c7e9 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 21 Sep 2016 14:56:01 +0200 Subject: [PATCH 23/33] qemu-iotests/067: Avoid blockdev-add with id We want to remove the 'id' option for blockdev-add. This removes one user of the option and makes it use only node names. In order to keep the test meaningful, some instances of query-block that want to check whether the node still exists and would now turn up empty must be converted to query-named-block-nodes (which also return the protocol level node, but that shouldn't hurt). Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- tests/qemu-iotests/067 | 6 +- tests/qemu-iotests/067.out | 211 ++++++++++++++++++++++--------------- 2 files changed, 131 insertions(+), 86 deletions(-) diff --git a/tests/qemu-iotests/067 b/tests/qemu-iotests/067 index c1df48eded..a12125bd46 100755 --- a/tests/qemu-iotests/067 +++ b/tests/qemu-iotests/067 @@ -121,7 +121,7 @@ run_qemu < Date: Wed, 21 Sep 2016 14:56:02 +0200 Subject: [PATCH 24/33] qemu-iotests/071: Avoid blockdev-add with id We want to remove the 'id' option for blockdev-add. This removes one user of the option and makes it use only node names. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- tests/qemu-iotests/071 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071 index bdfd91fef1..6d0864cff6 100755 --- a/tests/qemu-iotests/071 +++ b/tests/qemu-iotests/071 @@ -118,7 +118,7 @@ run_qemu < Date: Wed, 21 Sep 2016 14:56:03 +0200 Subject: [PATCH 25/33] qemu-iotests/081: Avoid blockdev-add with id We want to remove the 'id' option for blockdev-add. This removes one user of the option and makes it use only node names. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- tests/qemu-iotests/081 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081 index d89fabcdbd..0a809f3499 100755 --- a/tests/qemu-iotests/081 +++ b/tests/qemu-iotests/081 @@ -119,7 +119,7 @@ run_qemu < Date: Wed, 21 Sep 2016 14:56:04 +0200 Subject: [PATCH 26/33] qemu-iotests/087: Avoid blockdev-add with id We want to remove the 'id' option for blockdev-add. This removes one user of the option and makes it use only node names. The test cases that test conflicts between the 'id' option to blockdev-add and existing block devices or the 'node-name' of the same command can be removed because it won't be possible to specify this at the end of the series. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- tests/qemu-iotests/087 | 62 +++----------------------------------- tests/qemu-iotests/087.out | 6 +--- 2 files changed, 6 insertions(+), 62 deletions(-) diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087 index e7bca37efc..5c04577b36 100755 --- a/tests/qemu-iotests/087 +++ b/tests/qemu-iotests/087 @@ -77,50 +77,12 @@ echo echo === Duplicate ID === echo -run_qemu < Date: Wed, 21 Sep 2016 14:56:05 +0200 Subject: [PATCH 27/33] qemu-iotests/117: Avoid blockdev-add with id We want to remove the 'id' option for blockdev-add. This removes one user of the option and makes it use only node names. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- tests/qemu-iotests/117 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/117 b/tests/qemu-iotests/117 index 9385b3f8da..5b28039e17 100755 --- a/tests/qemu-iotests/117 +++ b/tests/qemu-iotests/117 @@ -52,14 +52,14 @@ _send_qemu_cmd $QEMU_HANDLE \ _send_qemu_cmd $QEMU_HANDLE \ "{ 'execute': 'blockdev-add', - 'arguments': { 'options': { 'id': 'protocol', + 'arguments': { 'options': { 'node-name': 'protocol', 'driver': 'file', 'filename': '$TEST_IMG' } } }" \ 'return' _send_qemu_cmd $QEMU_HANDLE \ "{ 'execute': 'blockdev-add', - 'arguments': { 'options': { 'id': 'format', + 'arguments': { 'options': { 'node-name': 'format', 'driver': '$IMGFMT', 'file': 'protocol' } } }" \ 'return' From e4fd2e9dfca0b592e329eec9ba753914e7941104 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 21 Sep 2016 14:56:06 +0200 Subject: [PATCH 28/33] qemu-iotests/118: Avoid blockdev-add with id We want to remove the 'id' option for blockdev-add. This removes one user of the option and makes it use only node names. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- tests/qemu-iotests/118 | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118 index 0380069ef6..e63a40fa94 100755 --- a/tests/qemu-iotests/118 +++ b/tests/qemu-iotests/118 @@ -648,13 +648,9 @@ class TestBlockJobsAfterCycle(ChangeBaseClass): qemu_img('create', '-f', iotests.imgfmt, old_img, '1M') self.vm = iotests.VM() + self.vm.add_drive_raw("id=drive0,driver=null-co,if=none") self.vm.launch() - result = self.vm.qmp('blockdev-add', - options={'id': 'drive0', - 'driver': 'null-co'}) - self.assert_qmp(result, 'return', {}) - result = self.vm.qmp('query-block') self.assert_qmp(result, 'return[0]/inserted/image/format', 'null-co') From eed875838e8d667e1fad5f6f5872c3d60ee266dd Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 21 Sep 2016 14:56:07 +0200 Subject: [PATCH 29/33] qemu-iotests/124: Avoid blockdev-add with id We want to remove the 'id' option for blockdev-add. This removes one user of the option and makes it use only node names. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- tests/qemu-iotests/124 | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124 index de7cdbe00e..2f0bc24cd0 100644 --- a/tests/qemu-iotests/124 +++ b/tests/qemu-iotests/124 @@ -49,8 +49,8 @@ def transaction_bitmap_clear(node, name, **kwargs): def transaction_drive_backup(device, target, **kwargs): - return transaction_action('drive-backup', device=device, target=target, - **kwargs) + return transaction_action('drive-backup', job_id=device, device=device, + target=target, **kwargs) class Bitmap: @@ -177,7 +177,8 @@ class TestIncrementalBackupBase(iotests.QMPTestCase): def create_anchor_backup(self, drive=None): if drive is None: drive = self.drives[-1] - res = self.do_qmp_backup(device=drive['id'], sync='full', + res = self.do_qmp_backup(job_id=drive['id'], + device=drive['id'], sync='full', format=drive['fmt'], target=drive['backup']) self.assertTrue(res) self.files.append(drive['backup']) @@ -188,7 +189,8 @@ class TestIncrementalBackupBase(iotests.QMPTestCase): if bitmap is None: bitmap = self.bitmaps[-1] _, reference = bitmap.last_target() - res = self.do_qmp_backup(device=bitmap.drive['id'], sync='full', + res = self.do_qmp_backup(job_id=bitmap.drive['id'], + device=bitmap.drive['id'], sync='full', format=bitmap.drive['fmt'], target=reference) self.assertTrue(res) @@ -221,7 +223,8 @@ class TestIncrementalBackupBase(iotests.QMPTestCase): parent, _ = bitmap.last_target() target = self.prepare_backup(bitmap, parent) - res = self.do_qmp_backup(device=bitmap.drive['id'], + res = self.do_qmp_backup(job_id=bitmap.drive['id'], + device=bitmap.drive['id'], sync='incremental', bitmap=bitmap.name, format=bitmap.drive['fmt'], target=target, mode='existing') @@ -414,7 +417,7 @@ class TestIncrementalBackup(TestIncrementalBackupBase): # Create a blkdebug interface to this img as 'drive1' result = self.vm.qmp('blockdev-add', options={ - 'id': drive1['id'], + 'node-name': drive1['id'], 'driver': drive1['fmt'], 'file': { 'driver': 'blkdebug', @@ -558,7 +561,7 @@ class TestIncrementalBackupBlkdebug(TestIncrementalBackupBase): drive0 = self.drives[0] result = self.vm.qmp('blockdev-add', options={ - 'id': drive0['id'], + 'node-name': drive0['id'], 'driver': drive0['fmt'], 'file': { 'driver': 'blkdebug', From 62acae8a9d20c872212aec59d4a0a634a97e87f0 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 21 Sep 2016 14:56:08 +0200 Subject: [PATCH 30/33] qemu-iotests/139: Avoid blockdev-add with id We want to remove the 'id' option for blockdev-add. This removes one user of the option and makes it use only node names. Some test cases that used to work with an unattached BlockBackend are removed, either because they don't make sense with an attached device or because the equivalent test case with an attached device already exists. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- tests/qemu-iotests/139 | 178 ++++++++++++------------------------- tests/qemu-iotests/139.out | 4 +- 2 files changed, 59 insertions(+), 123 deletions(-) diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139 index a4b969499c..47a4c26e29 100644 --- a/tests/qemu-iotests/139 +++ b/tests/qemu-iotests/139 @@ -31,6 +31,7 @@ class TestBlockdevDel(iotests.QMPTestCase): def setUp(self): iotests.qemu_img('create', '-f', iotests.imgfmt, base_img, '1M') self.vm = iotests.VM() + self.vm.add_device("virtio-scsi-pci,id=virtio-scsi") self.vm.launch() def tearDown(self): @@ -39,18 +40,6 @@ class TestBlockdevDel(iotests.QMPTestCase): if os.path.isfile(new_img): os.remove(new_img) - # Check whether a BlockBackend exists - def checkBlockBackend(self, backend, node, must_exist = True): - result = self.vm.qmp('query-block') - backends = filter(lambda x: x['device'] == backend, result['return']) - self.assertLessEqual(len(backends), 1) - self.assertEqual(must_exist, len(backends) == 1) - if must_exist: - if node: - self.assertEqual(backends[0]['inserted']['node-name'], node) - else: - self.assertFalse(backends[0].has_key('inserted')) - # Check whether a BlockDriverState exists def checkBlockDriverState(self, node, must_exist = True): result = self.vm.qmp('query-named-block-nodes') @@ -58,24 +47,6 @@ class TestBlockdevDel(iotests.QMPTestCase): self.assertLessEqual(len(nodes), 1) self.assertEqual(must_exist, len(nodes) == 1) - # Add a new BlockBackend (with its attached BlockDriverState) - def addBlockBackend(self, backend, node): - file_node = '%s_file' % node - self.checkBlockBackend(backend, node, False) - self.checkBlockDriverState(node, False) - self.checkBlockDriverState(file_node, False) - opts = {'driver': iotests.imgfmt, - 'id': backend, - 'node-name': node, - 'file': {'driver': 'file', - 'node-name': file_node, - 'filename': base_img}} - result = self.vm.qmp('blockdev-add', conv_keys = False, options = opts) - self.assert_qmp(result, 'return', {}) - self.checkBlockBackend(backend, node) - self.checkBlockDriverState(node) - self.checkBlockDriverState(file_node) - # Add a BlockDriverState without a BlockBackend def addBlockDriverState(self, node): file_node = '%s_file' % node @@ -105,23 +76,6 @@ class TestBlockdevDel(iotests.QMPTestCase): self.assert_qmp(result, 'return', {}) self.checkBlockDriverState(node) - # Delete a BlockBackend - def delBlockBackend(self, backend, node, expect_error = False, - destroys_media = True): - self.checkBlockBackend(backend, node) - if node: - self.checkBlockDriverState(node) - result = self.vm.qmp('x-blockdev-del', id = backend) - if expect_error: - self.assert_qmp(result, 'error/class', 'GenericError') - if node: - self.checkBlockDriverState(node) - else: - self.assert_qmp(result, 'return', {}) - if node: - self.checkBlockDriverState(node, not destroys_media) - self.checkBlockBackend(backend, node, must_exist = expect_error) - # Delete a BlockDriverState def delBlockDriverState(self, node, expect_error = False): self.checkBlockDriverState(node) @@ -133,51 +87,47 @@ class TestBlockdevDel(iotests.QMPTestCase): self.checkBlockDriverState(node, expect_error) # Add a device model - def addDeviceModel(self, device, backend): + def addDeviceModel(self, device, backend, driver = 'virtio-blk-pci'): result = self.vm.qmp('device_add', id = device, - driver = 'virtio-blk-pci', drive = backend) + driver = driver, drive = backend) self.assert_qmp(result, 'return', {}) # Delete a device model - def delDeviceModel(self, device): + def delDeviceModel(self, device, is_virtio_blk = True): result = self.vm.qmp('device_del', id = device) self.assert_qmp(result, 'return', {}) result = self.vm.qmp('system_reset') self.assert_qmp(result, 'return', {}) - device_path = '/machine/peripheral/%s/virtio-backend' % device - event = self.vm.event_wait(name="DEVICE_DELETED", - match={'data': {'path': device_path}}) - self.assertNotEqual(event, None) + if is_virtio_blk: + device_path = '/machine/peripheral/%s/virtio-backend' % device + event = self.vm.event_wait(name="DEVICE_DELETED", + match={'data': {'path': device_path}}) + self.assertNotEqual(event, None) event = self.vm.event_wait(name="DEVICE_DELETED", match={'data': {'device': device}}) self.assertNotEqual(event, None) # Remove a BlockDriverState - def ejectDrive(self, backend, node, expect_error = False, + def ejectDrive(self, device, node, expect_error = False, destroys_media = True): - self.checkBlockBackend(backend, node) self.checkBlockDriverState(node) - result = self.vm.qmp('eject', device = backend) + result = self.vm.qmp('eject', id = device) if expect_error: self.assert_qmp(result, 'error/class', 'GenericError') self.checkBlockDriverState(node) - self.checkBlockBackend(backend, node) else: self.assert_qmp(result, 'return', {}) self.checkBlockDriverState(node, not destroys_media) - self.checkBlockBackend(backend, None) # Insert a BlockDriverState - def insertDrive(self, backend, node): - self.checkBlockBackend(backend, None) + def insertDrive(self, device, node): self.checkBlockDriverState(node) result = self.vm.qmp('x-blockdev-insert-medium', - device = backend, node_name = node) + id = device, node_name = node) self.assert_qmp(result, 'return', {}) - self.checkBlockBackend(backend, node) self.checkBlockDriverState(node) # Create a snapshot using 'blockdev-snapshot-sync' @@ -204,26 +154,23 @@ class TestBlockdevDel(iotests.QMPTestCase): self.checkBlockDriverState(overlay) # Create a mirror - def createMirror(self, backend, node, new_node): - self.checkBlockBackend(backend, node) + def createMirror(self, node, new_node): self.checkBlockDriverState(new_node, False) - opts = {'device': backend, + opts = {'device': node, + 'job-id': node, 'target': new_img, 'node-name': new_node, 'sync': 'top', 'format': iotests.imgfmt} result = self.vm.qmp('drive-mirror', conv_keys=False, **opts) self.assert_qmp(result, 'return', {}) - self.checkBlockBackend(backend, node) self.checkBlockDriverState(new_node) # Complete an existing block job - def completeBlockJob(self, backend, node_before, node_after): - self.checkBlockBackend(backend, node_before) - result = self.vm.qmp('block-job-complete', device=backend) + def completeBlockJob(self, id, node_before, node_after): + result = self.vm.qmp('block-job-complete', device=id) self.assert_qmp(result, 'return', {}) - self.wait_until_completed(backend) - self.checkBlockBackend(backend, node_after) + self.wait_until_completed(id) # Add a BlkDebug node # Note that the purpose of this is to test the x-blockdev-del @@ -297,89 +244,78 @@ class TestBlockdevDel(iotests.QMPTestCase): # The tests start here # ######################## - def testWrongParameters(self): - self.addBlockBackend('drive0', 'node0') - result = self.vm.qmp('x-blockdev-del') - self.assert_qmp(result, 'error/class', 'GenericError') - result = self.vm.qmp('x-blockdev-del', id='drive0', node_name='node0') - self.assert_qmp(result, 'error/class', 'GenericError') - self.delBlockBackend('drive0', 'node0') - - def testBlockBackend(self): - self.addBlockBackend('drive0', 'node0') - # You cannot delete a BDS that is attached to a backend - self.delBlockDriverState('node0', expect_error = True) - self.delBlockBackend('drive0', 'node0') - def testBlockDriverState(self): self.addBlockDriverState('node0') # You cannot delete a file BDS directly self.delBlockDriverState('node0_file', expect_error = True) self.delBlockDriverState('node0') - def testEject(self): - self.addBlockBackend('drive0', 'node0') - self.ejectDrive('drive0', 'node0') - self.delBlockBackend('drive0', None) - def testDeviceModel(self): - self.addBlockBackend('drive0', 'node0') - self.addDeviceModel('device0', 'drive0') - self.ejectDrive('drive0', 'node0', expect_error = True) - self.delBlockBackend('drive0', 'node0', expect_error = True) + self.addBlockDriverState('node0') + self.addDeviceModel('device0', 'node0') + self.ejectDrive('device0', 'node0', expect_error = True) + self.delBlockDriverState('node0', expect_error = True) self.delDeviceModel('device0') - self.delBlockBackend('drive0', 'node0') + self.delBlockDriverState('node0') def testAttachMedia(self): # This creates a BlockBackend and removes its media - self.addBlockBackend('drive0', 'node0') - self.ejectDrive('drive0', 'node0') - # This creates a new BlockDriverState and inserts it into the backend + self.addBlockDriverState('node0') + self.addDeviceModel('device0', 'node0', 'scsi-cd') + self.ejectDrive('device0', 'node0', destroys_media = False) + self.delBlockDriverState('node0') + + # This creates a new BlockDriverState and inserts it into the device self.addBlockDriverState('node1') - self.insertDrive('drive0', 'node1') - # The backend can't be removed: the new BDS has an extra reference - self.delBlockBackend('drive0', 'node1', expect_error = True) + self.insertDrive('device0', 'node1') + # The node can't be removed: the new device has an extra reference self.delBlockDriverState('node1', expect_error = True) # The BDS still exists after being ejected, but now it can be removed - self.ejectDrive('drive0', 'node1', destroys_media = False) + self.ejectDrive('device0', 'node1', destroys_media = False) self.delBlockDriverState('node1') - self.delBlockBackend('drive0', None) + self.delDeviceModel('device0', False) def testSnapshotSync(self): - self.addBlockBackend('drive0', 'node0') + self.addBlockDriverState('node0') + self.addDeviceModel('device0', 'node0') self.createSnapshotSync('node0', 'overlay0') # This fails because node0 is now being used as a backing image self.delBlockDriverState('node0', expect_error = True) - # This succeeds because overlay0 only has the backend reference - self.delBlockBackend('drive0', 'overlay0') - self.checkBlockDriverState('node0', False) + self.delBlockDriverState('overlay0', expect_error = True) + # This succeeds because device0 only has the backend reference + self.delDeviceModel('device0') + # FIXME Would still be there if blockdev-snapshot-sync took a ref + self.checkBlockDriverState('overlay0', False) + self.delBlockDriverState('node0') def testSnapshot(self): - self.addBlockBackend('drive0', 'node0') + self.addBlockDriverState('node0') + self.addDeviceModel('device0', 'node0', 'scsi-cd') self.addBlockDriverStateOverlay('overlay0') self.createSnapshot('node0', 'overlay0') - self.delBlockBackend('drive0', 'overlay0', expect_error = True) self.delBlockDriverState('node0', expect_error = True) self.delBlockDriverState('overlay0', expect_error = True) - self.ejectDrive('drive0', 'overlay0', destroys_media = False) - self.delBlockBackend('drive0', None) + self.ejectDrive('device0', 'overlay0', destroys_media = False) self.delBlockDriverState('node0', expect_error = True) self.delBlockDriverState('overlay0') - self.checkBlockDriverState('node0', False) + self.delBlockDriverState('node0') def testMirror(self): - self.addBlockBackend('drive0', 'node0') - self.createMirror('drive0', 'node0', 'mirror0') + self.addBlockDriverState('node0') + self.addDeviceModel('device0', 'node0', 'scsi-cd') + self.createMirror('node0', 'mirror0') # The block job prevents removing the device - self.delBlockBackend('drive0', 'node0', expect_error = True) self.delBlockDriverState('node0', expect_error = True) self.delBlockDriverState('mirror0', expect_error = True) - self.wait_ready('drive0') - self.completeBlockJob('drive0', 'node0', 'mirror0') + self.wait_ready('node0') + self.completeBlockJob('node0', 'node0', 'mirror0') self.assert_no_active_block_jobs() - self.checkBlockDriverState('node0', False) - # This succeeds because the backend now points to mirror0 - self.delBlockBackend('drive0', 'mirror0') + # This succeeds because the device now points to mirror0 + self.delBlockDriverState('node0') + self.delBlockDriverState('mirror0', expect_error = True) + self.delDeviceModel('device0', False) + # FIXME mirror0 disappears, drive-mirror doesn't take a reference + #self.delBlockDriverState('mirror0') def testBlkDebug(self): self.addBlkDebug('debug0', 'node0') diff --git a/tests/qemu-iotests/139.out b/tests/qemu-iotests/139.out index 281b69efea..dae404e278 100644 --- a/tests/qemu-iotests/139.out +++ b/tests/qemu-iotests/139.out @@ -1,5 +1,5 @@ -............ +......... ---------------------------------------------------------------------- -Ran 12 tests +Ran 9 tests OK From e467da7b92ca207f02d29f52583a1097440a83cc Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 21 Sep 2016 14:56:09 +0200 Subject: [PATCH 31/33] block: Avoid printing NULL string in error messages Even for nodes that have a BlockBackend attached, bdrv_get_parent_name() can return NULL if the BB is anonymous (e.g. it belongs to a block job or a device that was created with a drive= option). Remove the information from the error message. The user probably knows already why the node is still in use. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- blockdev.c | 9 +++------ tests/qemu-iotests/085.out | 6 +++--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/blockdev.c b/blockdev.c index 405145ae95..17c2671c5e 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1803,8 +1803,7 @@ static void external_snapshot_prepare(BlkActionState *common, } if (bdrv_has_blk(state->new_bs)) { - error_setg(errp, "The snapshot is already in use by %s", - bdrv_get_parent_name(state->new_bs)); + error_setg(errp, "The snapshot is already in use"); return; } @@ -2532,8 +2531,7 @@ void qmp_x_blockdev_insert_medium(bool has_device, const char *device, } if (bdrv_has_blk(bs)) { - error_setg(errp, "Node '%s' is already in use by '%s'", node_name, - bdrv_get_parent_name(bs)); + error_setg(errp, "Node '%s' is already in use", node_name); return; } @@ -3941,8 +3939,7 @@ void qmp_x_blockdev_del(bool has_id, const char *id, return; } if (bdrv_has_blk(bs)) { - error_setg(errp, "Node %s is in use by %s", - node_name, bdrv_get_parent_name(bs)); + error_setg(errp, "Node %s is in use", node_name); return; } aio_context = bdrv_get_aio_context(bs); diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out index 01c78d6894..08e4bb7218 100644 --- a/tests/qemu-iotests/085.out +++ b/tests/qemu-iotests/085.out @@ -68,9 +68,9 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/ === Invalid command - snapshot node used as active layer === -{"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio0"}} -{"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio0"}} -{"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio1"}} +{"error": {"class": "GenericError", "desc": "The snapshot is already in use"}} +{"error": {"class": "GenericError", "desc": "The snapshot is already in use"}} +{"error": {"class": "GenericError", "desc": "The snapshot is already in use"}} === Invalid command - snapshot node used as backing hd === From 78645881508ebcfced97dc608b90179b264914e9 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 21 Sep 2016 14:56:10 +0200 Subject: [PATCH 32/33] qemu-iotests/141: Avoid blockdev-add with id We want to remove the 'id' option for blockdev-add. This removes one user of the option and makes it use only node names. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- tests/qemu-iotests/141 | 24 ++++++++++++++---------- tests/qemu-iotests/141.out | 24 ++++++++++++------------ 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141 index b2617e5e2b..c092d872dc 100755 --- a/tests/qemu-iotests/141 +++ b/tests/qemu-iotests/141 @@ -51,7 +51,7 @@ test_blockjob() "{'execute': 'blockdev-add', 'arguments': { 'options': { - 'id': 'drv0', + 'node-name': 'drv0', 'driver': '$IMGFMT', 'file': { 'driver': 'file', @@ -66,18 +66,18 @@ test_blockjob() # We want this to return an error because the block job is still running _send_qemu_cmd $QEMU_HANDLE \ - "{'execute': 'x-blockdev-remove-medium', - 'arguments': {'device': 'drv0'}}" \ + "{'execute': 'x-blockdev-del', + 'arguments': {'node-name': 'drv0'}}" \ 'error' _send_qemu_cmd $QEMU_HANDLE \ "{'execute': 'block-job-cancel', - 'arguments': {'device': 'drv0'}}" \ + 'arguments': {'device': 'job0'}}" \ "$3" _send_qemu_cmd $QEMU_HANDLE \ "{'execute': 'x-blockdev-del', - 'arguments': {'id': 'drv0'}}" \ + 'arguments': {'node-name': 'drv0'}}" \ 'return' } @@ -101,7 +101,8 @@ echo test_blockjob \ "{'execute': 'drive-backup', - 'arguments': {'device': 'drv0', + 'arguments': {'job-id': 'job0', + 'device': 'drv0', 'target': '$TEST_DIR/o.$IMGFMT', 'format': '$IMGFMT', 'sync': 'none'}}" \ @@ -117,7 +118,8 @@ echo test_blockjob \ "{'execute': 'drive-mirror', - 'arguments': {'device': 'drv0', + 'arguments': {'job-id': 'job0', + 'device': 'drv0', 'target': '$TEST_DIR/o.$IMGFMT', 'format': '$IMGFMT', 'sync': 'none'}}" \ @@ -134,7 +136,7 @@ echo test_blockjob \ "{'execute': 'block-commit', - 'arguments': {'device': 'drv0'}}" \ + 'arguments': {'job-id': 'job0', 'device': 'drv0'}}" \ 'BLOCK_JOB_READY' \ 'BLOCK_JOB_COMPLETED' @@ -150,7 +152,8 @@ $QEMU_IO -c 'write 0 1M' "$TEST_DIR/m.$IMGFMT" | _filter_qemu_io test_blockjob \ "{'execute': 'block-commit', - 'arguments': {'device': 'drv0', + 'arguments': {'job-id': 'job0', + 'device': 'drv0', 'top': '$TEST_DIR/m.$IMGFMT', 'speed': 1}}" \ 'return' \ @@ -172,7 +175,8 @@ $QEMU_IO -c 'write 0 1M' "$TEST_DIR/b.$IMGFMT" | _filter_qemu_io test_blockjob \ "{'execute': 'block-stream', - 'arguments': {'device': 'drv0', + 'arguments': {'job-id': 'job0', + 'device': 'drv0', 'speed': 1}}" \ 'return' \ 'BLOCK_JOB_CANCELLED' diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out index eaf1e603ed..195ca1a604 100644 --- a/tests/qemu-iotests/141.out +++ b/tests/qemu-iotests/141.out @@ -9,30 +9,30 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/m. {"return": {}} Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT {"return": {}} -{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: backup"}} +{"error": {"class": "GenericError", "desc": "Node drv0 is in use"}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 0, "speed": 0, "type": "backup"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "job0", "len": 1048576, "offset": 0, "speed": 0, "type": "backup"}} {"return": {}} === Testing drive-mirror === {"return": {}} Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}} {"return": {}} -{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: mirror"}} +{"error": {"class": "GenericError", "desc": "Node drv0 is in use"}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}} {"return": {}} === Testing active block-commit === {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}} {"return": {}} -{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}} +{"error": {"class": "GenericError", "desc": "Node drv0 is in use"}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}} {"return": {}} === Testing non-active block-commit === @@ -41,9 +41,9 @@ wrote 1048576/1048576 bytes at offset 0 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) {"return": {}} {"return": {}} -{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}} +{"error": {"class": "GenericError", "desc": "Node drv0 is in use"}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "commit"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "job0", "len": 1048576, "offset": 524288, "speed": 1, "type": "commit"}} {"return": {}} === Testing block-stream === @@ -52,8 +52,8 @@ wrote 1048576/1048576 bytes at offset 0 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) {"return": {}} {"return": {}} -{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: stream"}} +{"error": {"class": "GenericError", "desc": "Node drv0 is in use"}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "stream"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "job0", "len": 1048576, "offset": 524288, "speed": 1, "type": "stream"}} {"return": {}} *** done From 9ec8873e684c2dae6fadb3a801057c613ccd2a6b Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 21 Sep 2016 14:56:11 +0200 Subject: [PATCH 33/33] block: Remove BB interface from blockdev-add/del With this patch, blockdev-add always works on a node level, i.e. it creates a BDS, but no BB. Consequently, x-blockdev-del doesn't need the 'device' option any more, but 'node-name' becomes mandatory. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- blockdev.c | 128 +++++++++++-------------------------- docs/qmp-commands.txt | 24 ++----- qapi/block-core.json | 30 ++------- tests/qemu-iotests/087.out | 2 +- 4 files changed, 50 insertions(+), 134 deletions(-) diff --git a/blockdev.c b/blockdev.c index 17c2671c5e..29c6561fd8 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2844,7 +2844,7 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) bs = bdrv_find_node(id); if (bs) { - qmp_x_blockdev_del(false, NULL, true, id, &local_err); + qmp_x_blockdev_del(id, &local_err); if (local_err) { error_report_err(local_err); } @@ -3827,7 +3827,6 @@ out: void qmp_blockdev_add(BlockdevOptions *options, Error **errp) { BlockDriverState *bs; - BlockBackend *blk = NULL; QObject *obj; Visitor *v = qmp_output_visitor_new(&obj); QDict *qdict; @@ -3859,37 +3858,21 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) qdict_flatten(qdict); - if (options->has_id) { - blk = blockdev_init(NULL, qdict, &local_err); - if (local_err) { - error_propagate(errp, local_err); - goto fail; - } - - bs = blk_bs(blk); - } else { - if (!qdict_get_try_str(qdict, "node-name")) { - error_setg(errp, "'id' and/or 'node-name' need to be specified for " - "the root node"); - goto fail; - } - - bs = bds_tree_init(qdict, errp); - if (!bs) { - goto fail; - } - - QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list); + if (!qdict_get_try_str(qdict, "node-name")) { + error_setg(errp, "'node-name' must be specified for the root node"); + goto fail; } + bs = bds_tree_init(qdict, errp); + if (!bs) { + goto fail; + } + + QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list); + if (bs && bdrv_key_required(bs)) { - if (blk) { - monitor_remove_blk(blk); - blk_unref(blk); - } else { - QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list); - bdrv_unref(bs); - } + QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list); + bdrv_unref(bs); error_setg(errp, "blockdev-add doesn't support encrypted devices"); goto fail; } @@ -3898,81 +3881,42 @@ fail: visit_free(v); } -void qmp_x_blockdev_del(bool has_id, const char *id, - bool has_node_name, const char *node_name, Error **errp) +void qmp_x_blockdev_del(const char *node_name, Error **errp) { AioContext *aio_context; - BlockBackend *blk; BlockDriverState *bs; - if (has_id && has_node_name) { - error_setg(errp, "Only one of id and node-name must be specified"); - return; - } else if (!has_id && !has_node_name) { - error_setg(errp, "No block device specified"); + bs = bdrv_find_node(node_name); + if (!bs) { + error_setg(errp, "Cannot find node %s", node_name); return; } - - if (has_id) { - /* blk_by_name() never returns a BB that is not owned by the monitor */ - blk = blk_by_name(id); - if (!blk) { - error_setg(errp, "Cannot find block backend %s", id); - return; - } - if (blk_legacy_dinfo(blk)) { - error_setg(errp, "Deleting block backend added with drive-add" - " is not supported"); - return; - } - if (blk_get_refcnt(blk) > 1) { - error_setg(errp, "Block backend %s is in use", id); - return; - } - bs = blk_bs(blk); - aio_context = blk_get_aio_context(blk); - } else { - blk = NULL; - bs = bdrv_find_node(node_name); - if (!bs) { - error_setg(errp, "Cannot find node %s", node_name); - return; - } - if (bdrv_has_blk(bs)) { - error_setg(errp, "Node %s is in use", node_name); - return; - } - aio_context = bdrv_get_aio_context(bs); + if (bdrv_has_blk(bs)) { + error_setg(errp, "Node %s is in use", node_name); + return; } - + aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); - if (bs) { - if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, errp)) { - goto out; - } - - if (!blk && !QTAILQ_IN_USE(bs, monitor_list)) { - error_setg(errp, "Node %s is not owned by the monitor", - bs->node_name); - goto out; - } - - if (bs->refcnt > 1) { - error_setg(errp, "Block device %s is in use", - bdrv_get_device_or_node_name(bs)); - goto out; - } + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, errp)) { + goto out; } - if (blk) { - monitor_remove_blk(blk); - blk_unref(blk); - } else { - QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list); - bdrv_unref(bs); + if (!bs->monitor_list.tqe_prev) { + error_setg(errp, "Node %s is not owned by the monitor", + bs->node_name); + goto out; } + if (bs->refcnt > 1) { + error_setg(errp, "Block device %s is in use", + bdrv_get_device_or_node_name(bs)); + goto out; + } + + QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list); + bdrv_unref(bs); + out: aio_context_release(aio_context); } diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt index 41f56981af..e0adcebc67 100644 --- a/docs/qmp-commands.txt +++ b/docs/qmp-commands.txt @@ -3141,7 +3141,7 @@ Example (2): "arguments": { "options": { "driver": "qcow2", - "id": "my_disk", + "node-name": "my_disk", "discard": "unmap", "cache": { "direct": true, @@ -3168,18 +3168,9 @@ x-blockdev-del ------------ Since 2.5 -Deletes a block device thas has been added using blockdev-add. -The selected device can be either a block backend or a graph node. - -In the former case the backend will be destroyed, along with its -inserted medium if there's any. The command will fail if the backend -or its medium are in use. - -In the latter case the node will be destroyed. The command will fail -if the node is attached to a block backend or is otherwise being -used. - -One of "id" or "node-name" must be specified, but not both. +Deletes a block device that has been added using blockdev-add. +The command will fail if the node is attached to a device or is +otherwise being used. This command is still a work in progress and is considered experimental. Stay away from it unless you want to help with its @@ -3187,8 +3178,7 @@ development. Arguments: -- "id": Name of the block backend device to delete (json-string, optional) -- "node-name": Name of the graph node to delete (json-string, optional) +- "node-name": Name of the graph node to delete (json-string) Example: @@ -3196,7 +3186,7 @@ Example: "arguments": { "options": { "driver": "qcow2", - "id": "drive0", + "node-name": "node0", "file": { "driver": "file", "filename": "test.qcow2" @@ -3208,7 +3198,7 @@ Example: <- { "return": {} } -> { "execute": "x-blockdev-del", - "arguments": { "id": "drive0" } + "arguments": { "node-name": "node0" } } <- { "return": {} } diff --git a/qapi/block-core.json b/qapi/block-core.json index 5f04dab027..92193ab0a1 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2217,13 +2217,8 @@ # block devices, independent of the block driver: # # @driver: block driver name -# @id: #optional id by which the new block device can be referred to. -# This option is only allowed on the top level of blockdev-add. -# A BlockBackend will be created by blockdev-add if and only if -# this option is given. -# @node-name: #optional the name of a block driver state node (Since 2.0). -# This option is required on the top level of blockdev-add if -# the @id option is not given there. +# @node-name: #optional the node name of the new node (Since 2.0). +# This option is required on the top level of blockdev-add. # @discard: #optional discard-related options (default: ignore) # @cache: #optional cache-related options # @aio: #optional AIO backend (default: threads) @@ -2238,8 +2233,6 @@ ## { 'union': 'BlockdevOptions', 'base': { 'driver': 'BlockdevDriver', -# TODO 'id' is a BB-level option, remove it - '*id': 'str', '*node-name': 'str', '*discard': 'BlockdevDiscardOptions', '*cache': 'BlockdevCacheOptions', @@ -2323,29 +2316,18 @@ # @x-blockdev-del: # # Deletes a block device that has been added using blockdev-add. -# The selected device can be either a block backend or a graph node. -# -# In the former case the backend will be destroyed, along with its -# inserted medium if there's any. The command will fail if the backend -# or its medium are in use. -# -# In the latter case the node will be destroyed. The command will fail -# if the node is attached to a block backend or is otherwise being -# used. -# -# One of @id or @node-name must be specified, but not both. +# The command will fail if the node is attached to a device or is +# otherwise being used. # # This command is still a work in progress and is considered # experimental. Stay away from it unless you want to help with its # development. # -# @id: #optional Name of the block backend device to delete. -# -# @node-name: #optional Name of the graph node to delete. +# @node-name: Name of the graph node to delete. # # Since: 2.5 ## -{ 'command': 'x-blockdev-del', 'data': { '*id': 'str', '*node-name': 'str' } } +{ 'command': 'x-blockdev-del', 'data': { 'node-name': 'str' } } ## # @blockdev-open-tray: diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out index f2d6f96805..bef68626c8 100644 --- a/tests/qemu-iotests/087.out +++ b/tests/qemu-iotests/087.out @@ -6,7 +6,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 Testing: QMP_VERSION {"return": {}} -{"error": {"class": "GenericError", "desc": "'id' and/or 'node-name' need to be specified for the root node"}} +{"error": {"class": "GenericError", "desc": "'node-name' must be specified for the root node"}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"}