1 | The following changes since commit ba29883206d92a29ad5a466e679ccfc2ee6132ef: | 1 | The following changes since commit 42f6c9179be4401974dd3a75ee72defd16b5092d: |
---|---|---|---|
2 | 2 | ||
3 | Merge remote-tracking branch 'remotes/borntraeger/tags/s390x-20200310' into staging (2020-03-10 16:50:28 +0000) | 3 | Merge tag 'pull-ppc-20211112' of https://github.com/legoater/qemu into staging (2021-11-12 12:28:25 +0100) |
4 | 4 | ||
5 | are available in the Git repository at: | 5 | are available in the Git repository at: |
6 | 6 | ||
7 | git://repo.or.cz/qemu/kevin.git tags/for-upstream | 7 | git://repo.or.cz/qemu/kevin.git tags/for-upstream |
8 | 8 | ||
9 | for you to fetch changes up to 8bb3b023f2055054ee119cb45b42d2b14be7fc8a: | 9 | for you to fetch changes up to 7461272c5f6032436ef9032c091c0118539483e4: |
10 | 10 | ||
11 | qemu-iotests: adding LUKS cleanup for non-UTF8 secret error (2020-03-11 15:54:38 +0100) | 11 | softmmu/qdev-monitor: fix use-after-free in qdev_set_id() (2021-11-15 15:49:46 +0100) |
12 | 12 | ||
13 | ---------------------------------------------------------------- | 13 | ---------------------------------------------------------------- |
14 | Block layer patches: | 14 | Block layer patches |
15 | 15 | ||
16 | - Relax restrictions for blockdev-snapshot (allows libvirt to do live | 16 | - Fixes to image streaming job and block layer reconfiguration to make |
17 | storage migration with blockdev-mirror) | 17 | iotest 030 pass again |
18 | - luks: Delete created files when block_crypto_co_create_opts_luks fails | 18 | - docs: Deprecate incorrectly typed device_add arguments |
19 | - Fix memleaks in qmp_object_add | 19 | - file-posix: Fix alignment after reopen changing O_DIRECT |
20 | 20 | ||
21 | ---------------------------------------------------------------- | 21 | ---------------------------------------------------------------- |
22 | Daniel Henrique Barboza (4): | 22 | Hanna Reitz (10): |
23 | block: introducing 'bdrv_co_delete_file' interface | 23 | stream: Traverse graph after modification |
24 | block.c: adding bdrv_co_delete_file | 24 | block: Manipulate children list in .attach/.detach |
25 | crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails | 25 | block: Unite remove_empty_child and child_free |
26 | qemu-iotests: adding LUKS cleanup for non-UTF8 secret error | 26 | block: Drop detached child from ignore list |
27 | block: Pass BdrvChild ** to replace_child_noperm | ||
28 | block: Restructure remove_file_or_backing_child() | ||
29 | transactions: Invoke clean() after everything else | ||
30 | block: Let replace_child_tran keep indirect pointer | ||
31 | block: Let replace_child_noperm free children | ||
32 | iotests/030: Unthrottle parallel jobs in reverse | ||
27 | 33 | ||
28 | Kevin Wolf (6): | 34 | Kevin Wolf (2): |
29 | block: Make bdrv_get_cumulative_perm() public | 35 | docs: Deprecate incorrectly typed device_add arguments |
30 | block: Relax restrictions for blockdev-snapshot | 36 | file-posix: Fix alignment after reopen changing O_DIRECT |
31 | iotests: Fix run_job() with use_log=False | ||
32 | iotests: Test mirror with temporarily disabled target backing file | ||
33 | block: Fix cross-AioContext blockdev-snapshot | ||
34 | iotests: Add iothread cases to 155 | ||
35 | 37 | ||
36 | Pan Nengyuan (1): | 38 | Stefan Hajnoczi (1): |
37 | qom-qmp-cmds: fix two memleaks in qmp_object_add | 39 | softmmu/qdev-monitor: fix use-after-free in qdev_set_id() |
38 | 40 | ||
39 | Peter Krempa (1): | 41 | docs/about/deprecated.rst | 14 +++ |
40 | qapi: Add '@allow-write-only-overlay' feature for 'blockdev-snapshot' | 42 | include/qemu/transactions.h | 3 + |
41 | 43 | block.c | 233 +++++++++++++++++++++++++++++++++----------- | |
42 | Philippe Mathieu-Daudé (1): | 44 | block/file-posix.c | 20 +++- |
43 | tests/qemu-iotests: Fix socket_scm_helper build path | 45 | block/stream.c | 7 +- |
44 | 46 | softmmu/qdev-monitor.c | 2 +- | |
45 | qapi/block-core.json | 9 ++++- | 47 | util/transactions.c | 8 +- |
46 | include/block/block.h | 1 + | 48 | tests/qemu-iotests/030 | 11 ++- |
47 | include/block/block_int.h | 7 ++++ | 49 | tests/qemu-iotests/142 | 22 +++++ |
48 | block.c | 33 ++++++++++++++-- | 50 | tests/qemu-iotests/142.out | 15 +++ |
49 | block/crypto.c | 18 +++++++++ | 51 | 10 files changed, 269 insertions(+), 66 deletions(-) |
50 | block/file-posix.c | 23 +++++++++++ | ||
51 | blockdev.c | 30 ++++----------- | ||
52 | qom/qom-qmp-cmds.c | 16 +++----- | ||
53 | tests/qemu-iotests/iotests.py | 5 ++- | ||
54 | tests/Makefile.include | 1 + | ||
55 | tests/qemu-iotests/085.out | 4 +- | ||
56 | tests/qemu-iotests/155 | 88 ++++++++++++++++++++++++++++++++++++------- | ||
57 | tests/qemu-iotests/155.out | 4 +- | ||
58 | tests/qemu-iotests/282 | 67 ++++++++++++++++++++++++++++++++ | ||
59 | tests/qemu-iotests/282.out | 11 ++++++ | ||
60 | tests/qemu-iotests/group | 1 + | ||
61 | tests/qtest/Makefile.include | 1 - | ||
62 | 17 files changed, 262 insertions(+), 57 deletions(-) | ||
63 | create mode 100755 tests/qemu-iotests/282 | ||
64 | create mode 100644 tests/qemu-iotests/282.out | ||
65 | 52 | ||
66 | 53 | diff view generated by jsdifflib |
1 | From: Daniel Henrique Barboza <danielhb413@gmail.com> | 1 | From: Hanna Reitz <hreitz@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | This patch adds a new test file to exercise the case where | 3 | bdrv_cor_filter_drop() modifies the block graph. That means that other |
4 | qemu-img fails to complete for the LUKS format when a non-UTF8 | 4 | parties can also modify the block graph before it returns. Therefore, |
5 | secret is used. | 5 | we cannot assume that the result of a graph traversal we did before |
6 | remains valid afterwards. | ||
6 | 7 | ||
7 | Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> | 8 | We should thus fetch `base` and `unfiltered_base` afterwards instead of |
8 | Message-Id: <20200130213907.2830642-5-danielhb413@gmail.com> | 9 | before. |
10 | |||
11 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
12 | Reviewed-by: Kevin Wolf <kwolf@redhat.com> | ||
13 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
14 | Message-Id: <20211111120829.81329-2-hreitz@redhat.com> | ||
9 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 15 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
10 | --- | 16 | --- |
11 | tests/qemu-iotests/282 | 67 ++++++++++++++++++++++++++++++++++++++ | 17 | block/stream.c | 7 +++++-- |
12 | tests/qemu-iotests/282.out | 11 +++++++ | 18 | 1 file changed, 5 insertions(+), 2 deletions(-) |
13 | tests/qemu-iotests/group | 1 + | ||
14 | 3 files changed, 79 insertions(+) | ||
15 | create mode 100755 tests/qemu-iotests/282 | ||
16 | create mode 100644 tests/qemu-iotests/282.out | ||
17 | 19 | ||
18 | diff --git a/tests/qemu-iotests/282 b/tests/qemu-iotests/282 | 20 | diff --git a/block/stream.c b/block/stream.c |
19 | new file mode 100755 | 21 | index XXXXXXX..XXXXXXX 100644 |
20 | index XXXXXXX..XXXXXXX | 22 | --- a/block/stream.c |
21 | --- /dev/null | 23 | +++ b/block/stream.c |
22 | +++ b/tests/qemu-iotests/282 | 24 | @@ -XXX,XX +XXX,XX @@ static int stream_prepare(Job *job) |
23 | @@ -XXX,XX +XXX,XX @@ | 25 | { |
24 | +#!/usr/bin/env bash | 26 | StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); |
25 | +# | 27 | BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs); |
26 | +# Test qemu-img file cleanup for LUKS when using a non-UTF8 secret | 28 | - BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base); |
27 | +# | 29 | - BlockDriverState *unfiltered_base = bdrv_skip_filters(base); |
28 | +# Copyright (C) 2020, IBM Corporation. | 30 | + BlockDriverState *base; |
29 | +# | 31 | + BlockDriverState *unfiltered_base; |
30 | +# This program is free software; you can redistribute it and/or modify | 32 | Error *local_err = NULL; |
31 | +# it under the terms of the GNU General Public License as published by | 33 | int ret = 0; |
32 | +# the Free Software Foundation; either version 2 of the License, or | 34 | |
33 | +# (at your option) any later version. | 35 | @@ -XXX,XX +XXX,XX @@ static int stream_prepare(Job *job) |
34 | +# | 36 | bdrv_cor_filter_drop(s->cor_filter_bs); |
35 | +# This program is distributed in the hope that it will be useful, | 37 | s->cor_filter_bs = NULL; |
36 | +# but WITHOUT ANY WARRANTY; without even the implied warranty of | 38 | |
37 | +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | 39 | + base = bdrv_filter_or_cow_bs(s->above_base); |
38 | +# GNU General Public License for more details. | 40 | + unfiltered_base = bdrv_skip_filters(base); |
39 | +# | ||
40 | +# You should have received a copy of the GNU General Public License | ||
41 | +# along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
42 | +# | ||
43 | + | 41 | + |
44 | +seq=`basename $0` | 42 | if (bdrv_cow_child(unfiltered_bs)) { |
45 | +echo "QA output created by $seq" | 43 | const char *base_id = NULL, *base_fmt = NULL; |
46 | + | 44 | if (unfiltered_base) { |
47 | +status=1 # failure is the default! | ||
48 | +TEST_IMAGE_FILE='vol.img' | ||
49 | + | ||
50 | +_cleanup() | ||
51 | +{ | ||
52 | + _cleanup_test_img | ||
53 | + rm non_utf8_secret | ||
54 | + rm -f $TEST_IMAGE_FILE | ||
55 | +} | ||
56 | +trap "_cleanup; exit \$status" 0 1 2 3 15 | ||
57 | + | ||
58 | +# get standard environment, filters and checks | ||
59 | +. ./common.rc | ||
60 | +. ./common.filter | ||
61 | + | ||
62 | +_supported_fmt luks | ||
63 | +_supported_proto generic | ||
64 | +_unsupported_proto vxhs | ||
65 | + | ||
66 | +echo "== Create non-UTF8 secret ==" | ||
67 | +echo -n -e '\x3a\x3c\x3b\xff' > non_utf8_secret | ||
68 | +SECRET="secret,id=sec0,file=non_utf8_secret" | ||
69 | + | ||
70 | +echo "== Throws an error because of invalid UTF-8 secret ==" | ||
71 | +$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0" $TEST_IMAGE_FILE 4M | ||
72 | + | ||
73 | +echo "== Image file should not exist after the error ==" | ||
74 | +if test -f "$TEST_IMAGE_FILE"; then | ||
75 | + exit 1 | ||
76 | +fi | ||
77 | + | ||
78 | +echo "== Create a stub image file and run qemu-img again ==" | ||
79 | +touch $TEST_IMAGE_FILE | ||
80 | +$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0" $TEST_IMAGE_FILE 4M | ||
81 | + | ||
82 | +echo "== Pre-existing image file should also be deleted after the error ==" | ||
83 | +if test -f "$TEST_IMAGE_FILE"; then | ||
84 | + exit 1 | ||
85 | +fi | ||
86 | + | ||
87 | +# success, all done | ||
88 | +echo "*** done" | ||
89 | +rm -f $seq.full | ||
90 | +status=0 | ||
91 | diff --git a/tests/qemu-iotests/282.out b/tests/qemu-iotests/282.out | ||
92 | new file mode 100644 | ||
93 | index XXXXXXX..XXXXXXX | ||
94 | --- /dev/null | ||
95 | +++ b/tests/qemu-iotests/282.out | ||
96 | @@ -XXX,XX +XXX,XX @@ | ||
97 | +QA output created by 282 | ||
98 | +== Create non-UTF8 secret == | ||
99 | +== Throws an error because of invalid UTF-8 secret == | ||
100 | +qemu-img: vol.img: Data from secret sec0 is not valid UTF-8 | ||
101 | +Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0 | ||
102 | +== Image file should not exist after the error == | ||
103 | +== Create a stub image file and run qemu-img again == | ||
104 | +qemu-img: vol.img: Data from secret sec0 is not valid UTF-8 | ||
105 | +Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0 | ||
106 | +== Pre-existing image file should also be deleted after the error == | ||
107 | + *** done | ||
108 | diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group | ||
109 | index XXXXXXX..XXXXXXX 100644 | ||
110 | --- a/tests/qemu-iotests/group | ||
111 | +++ b/tests/qemu-iotests/group | ||
112 | @@ -XXX,XX +XXX,XX @@ | ||
113 | 279 rw backing quick | ||
114 | 280 rw migration quick | ||
115 | 281 rw quick | ||
116 | +282 rw img quick | ||
117 | 283 auto quick | ||
118 | 284 rw | ||
119 | 286 rw quick | ||
120 | -- | 45 | -- |
121 | 2.20.1 | 46 | 2.31.1 |
122 | 47 | ||
123 | 48 | diff view generated by jsdifflib |
1 | From: Philippe Mathieu-Daudé <philmd@redhat.com> | 1 | From: Hanna Reitz <hreitz@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | The socket_scm_helper path got corrupted during the mechanical | 3 | The children list is specific to BDS parents. We should not modify it |
4 | refactor moving the qtests files into their own sub-directory. | 4 | in the general children modification code, but let BDS parents deal with |
5 | it in their .attach() and .detach() methods. | ||
5 | 6 | ||
6 | Fixes: 1e8a1fae7 ("test: Move qtests to a separate directory") | 7 | This also has the advantage that a BdrvChild is removed from the |
7 | Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> | 8 | children list before its .bs pointer can become NULL. BDS parents |
8 | Message-Id: <20200306165751.18986-1-philmd@redhat.com> | 9 | generally assume that their children's .bs pointer is never NULL, so |
9 | Reviewed-by: Laurent Vivier <laurent@vivier.eu> | 10 | this is actually a bug fix. |
11 | |||
12 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
13 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
14 | Message-Id: <20211111120829.81329-3-hreitz@redhat.com> | ||
10 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 15 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
11 | --- | 16 | --- |
12 | tests/Makefile.include | 1 + | 17 | block.c | 14 +++++--------- |
13 | tests/qtest/Makefile.include | 1 - | 18 | 1 file changed, 5 insertions(+), 9 deletions(-) |
14 | 2 files changed, 1 insertion(+), 1 deletion(-) | ||
15 | 19 | ||
16 | diff --git a/tests/Makefile.include b/tests/Makefile.include | 20 | diff --git a/block.c b/block.c |
17 | index XXXXXXX..XXXXXXX 100644 | 21 | index XXXXXXX..XXXXXXX 100644 |
18 | --- a/tests/Makefile.include | 22 | --- a/block.c |
19 | +++ b/tests/Makefile.include | 23 | +++ b/block.c |
20 | @@ -XXX,XX +XXX,XX @@ include $(SRC_PATH)/tests/qtest/Makefile.include | 24 | @@ -XXX,XX +XXX,XX @@ static void bdrv_child_cb_attach(BdrvChild *child) |
21 | tests/test-qga$(EXESUF): qemu-ga$(EXESUF) | 25 | { |
22 | tests/test-qga$(EXESUF): tests/test-qga.o $(qtest-obj-y) | 26 | BlockDriverState *bs = child->opaque; |
23 | tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o $(test-util-obj-y) libvhost-user.a | 27 | |
24 | +tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o | 28 | + QLIST_INSERT_HEAD(&bs->children, child, next); |
25 | 29 | + | |
26 | SPEED = quick | 30 | if (child->role & BDRV_CHILD_COW) { |
27 | 31 | bdrv_backing_attach(child); | |
28 | diff --git a/tests/qtest/Makefile.include b/tests/qtest/Makefile.include | 32 | } |
29 | index XXXXXXX..XXXXXXX 100644 | 33 | @@ -XXX,XX +XXX,XX @@ static void bdrv_child_cb_detach(BdrvChild *child) |
30 | --- a/tests/qtest/Makefile.include | 34 | } |
31 | +++ b/tests/qtest/Makefile.include | 35 | |
32 | @@ -XXX,XX +XXX,XX @@ tests/qtest/usb-hcd-ehci-test$(EXESUF): tests/qtest/usb-hcd-ehci-test.o $(libqos | 36 | bdrv_unapply_subtree_drain(child, bs); |
33 | tests/qtest/usb-hcd-xhci-test$(EXESUF): tests/qtest/usb-hcd-xhci-test.o $(libqos-usb-obj-y) | 37 | + |
34 | tests/qtest/cpu-plug-test$(EXESUF): tests/qtest/cpu-plug-test.o | 38 | + QLIST_REMOVE(child, next); |
35 | tests/qtest/migration-test$(EXESUF): tests/qtest/migration-test.o tests/qtest/migration-helpers.o | 39 | } |
36 | -tests/qtest/qemu-iotests/qtest/socket_scm_helper$(EXESUF): tests/qtest/qemu-iotests/qtest/socket_scm_helper.o | 40 | |
37 | tests/qtest/test-netfilter$(EXESUF): tests/qtest/test-netfilter.o $(qtest-obj-y) | 41 | static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base, |
38 | tests/qtest/test-filter-mirror$(EXESUF): tests/qtest/test-filter-mirror.o $(qtest-obj-y) | 42 | @@ -XXX,XX +XXX,XX @@ static void bdrv_child_free(void *opaque) |
39 | tests/qtest/test-filter-redirector$(EXESUF): tests/qtest/test-filter-redirector.o $(qtest-obj-y) | 43 | static void bdrv_remove_empty_child(BdrvChild *child) |
44 | { | ||
45 | assert(!child->bs); | ||
46 | - QLIST_SAFE_REMOVE(child, next); | ||
47 | + assert(!child->next.le_prev); /* not in children list */ | ||
48 | bdrv_child_free(child); | ||
49 | } | ||
50 | |||
51 | @@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, | ||
52 | return ret; | ||
53 | } | ||
54 | |||
55 | - QLIST_INSERT_HEAD(&parent_bs->children, *child, next); | ||
56 | - /* | ||
57 | - * child is removed in bdrv_attach_child_common_abort(), so don't care to | ||
58 | - * abort this change separately. | ||
59 | - */ | ||
60 | - | ||
61 | return 0; | ||
62 | } | ||
63 | |||
64 | @@ -XXX,XX +XXX,XX @@ static void bdrv_remove_filter_or_cow_child_abort(void *opaque) | ||
65 | BdrvRemoveFilterOrCowChild *s = opaque; | ||
66 | BlockDriverState *parent_bs = s->child->opaque; | ||
67 | |||
68 | - QLIST_INSERT_HEAD(&parent_bs->children, s->child, next); | ||
69 | if (s->is_backing) { | ||
70 | parent_bs->backing = s->child; | ||
71 | } else { | ||
72 | @@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, | ||
73 | }; | ||
74 | tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s); | ||
75 | |||
76 | - QLIST_SAFE_REMOVE(child, next); | ||
77 | if (s->is_backing) { | ||
78 | bs->backing = NULL; | ||
79 | } else { | ||
40 | -- | 80 | -- |
41 | 2.20.1 | 81 | 2.31.1 |
42 | 82 | ||
43 | 83 | diff view generated by jsdifflib |
1 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 1 | From: Hanna Reitz <hreitz@redhat.com> |
---|---|---|---|
2 | Message-Id: <20200310113831.27293-2-kwolf@redhat.com> | 2 | |
3 | Reviewed-by: Peter Krempa <pkrempa@redhat.com> | 3 | Now that bdrv_remove_empty_child() no longer removes the child from the |
4 | parent's children list but only checks that it is not in such a list, it | ||
5 | is only a wrapper around bdrv_child_free() that checks that the child is | ||
6 | empty and unused. That should apply to all children that we free, so | ||
7 | put those checks into bdrv_child_free() and drop | ||
8 | bdrv_remove_empty_child(). | ||
9 | |||
10 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
11 | Reviewed-by: Kevin Wolf <kwolf@redhat.com> | ||
12 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
13 | Message-Id: <20211111120829.81329-4-hreitz@redhat.com> | ||
4 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 14 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
5 | --- | 15 | --- |
6 | include/block/block_int.h | 3 +++ | 16 | block.c | 26 +++++++++++++------------- |
7 | block.c | 6 ++---- | 17 | 1 file changed, 13 insertions(+), 13 deletions(-) |
8 | 2 files changed, 5 insertions(+), 4 deletions(-) | ||
9 | 18 | ||
10 | diff --git a/include/block/block_int.h b/include/block/block_int.h | ||
11 | index XXXXXXX..XXXXXXX 100644 | ||
12 | --- a/include/block/block_int.h | ||
13 | +++ b/include/block/block_int.h | ||
14 | @@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, | ||
15 | void *opaque, Error **errp); | ||
16 | void bdrv_root_unref_child(BdrvChild *child); | ||
17 | |||
18 | +void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm, | ||
19 | + uint64_t *shared_perm); | ||
20 | + | ||
21 | /** | ||
22 | * Sets a BdrvChild's permissions. Avoid if the parent is a BDS; use | ||
23 | * bdrv_child_refresh_perms() instead and make the parent's | ||
24 | diff --git a/block.c b/block.c | 19 | diff --git a/block.c b/block.c |
25 | index XXXXXXX..XXXXXXX 100644 | 20 | index XXXXXXX..XXXXXXX 100644 |
26 | --- a/block.c | 21 | --- a/block.c |
27 | +++ b/block.c | 22 | +++ b/block.c |
28 | @@ -XXX,XX +XXX,XX @@ static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q, | 23 | @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild *child, |
29 | bool *tighten_restrictions, Error **errp); | ||
30 | static void bdrv_child_abort_perm_update(BdrvChild *c); | ||
31 | static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared); | ||
32 | -static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm, | ||
33 | - uint64_t *shared_perm); | ||
34 | |||
35 | typedef struct BlockReopenQueueEntry { | ||
36 | bool prepared; | ||
37 | @@ -XXX,XX +XXX,XX @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms, | ||
38 | } | 24 | } |
39 | } | 25 | } |
40 | 26 | ||
41 | -static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm, | 27 | -static void bdrv_child_free(void *opaque) |
42 | - uint64_t *shared_perm) | 28 | -{ |
43 | +void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm, | 29 | - BdrvChild *c = opaque; |
44 | + uint64_t *shared_perm) | 30 | - |
31 | - g_free(c->name); | ||
32 | - g_free(c); | ||
33 | -} | ||
34 | - | ||
35 | -static void bdrv_remove_empty_child(BdrvChild *child) | ||
36 | +/** | ||
37 | + * Free the given @child. | ||
38 | + * | ||
39 | + * The child must be empty (i.e. `child->bs == NULL`) and it must be | ||
40 | + * unused (i.e. not in a children list). | ||
41 | + */ | ||
42 | +static void bdrv_child_free(BdrvChild *child) | ||
45 | { | 43 | { |
46 | BdrvChild *c; | 44 | assert(!child->bs); |
47 | uint64_t cumulative_perms = 0; | 45 | assert(!child->next.le_prev); /* not in children list */ |
46 | - bdrv_child_free(child); | ||
47 | + | ||
48 | + g_free(child->name); | ||
49 | + g_free(child); | ||
50 | } | ||
51 | |||
52 | typedef struct BdrvAttachChildCommonState { | ||
53 | @@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque) | ||
54 | } | ||
55 | |||
56 | bdrv_unref(bs); | ||
57 | - bdrv_remove_empty_child(child); | ||
58 | + bdrv_child_free(child); | ||
59 | *s->child = NULL; | ||
60 | } | ||
61 | |||
62 | @@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_common(BlockDriverState *child_bs, | ||
63 | |||
64 | if (ret < 0) { | ||
65 | error_propagate(errp, local_err); | ||
66 | - bdrv_remove_empty_child(new_child); | ||
67 | + bdrv_child_free(new_child); | ||
68 | return ret; | ||
69 | } | ||
70 | } | ||
71 | @@ -XXX,XX +XXX,XX @@ static void bdrv_detach_child(BdrvChild *child) | ||
72 | BlockDriverState *old_bs = child->bs; | ||
73 | |||
74 | bdrv_replace_child_noperm(child, NULL); | ||
75 | - bdrv_remove_empty_child(child); | ||
76 | + bdrv_child_free(child); | ||
77 | |||
78 | if (old_bs) { | ||
79 | /* | ||
48 | -- | 80 | -- |
49 | 2.20.1 | 81 | 2.31.1 |
50 | 82 | ||
51 | 83 | diff view generated by jsdifflib |
1 | external_snapshot_prepare() tries to move the overlay to the AioContext | 1 | From: Hanna Reitz <hreitz@redhat.com> |
---|---|---|---|
2 | of the backing file (the snapshotted node). However, it's possible that | ||
3 | this doesn't work, but the backing file can instead be moved to the | ||
4 | overlay's AioContext (e.g. opening the backing chain for a mirror | ||
5 | target). | ||
6 | 2 | ||
7 | bdrv_append() already indirectly uses bdrv_attach_node(), which takes | 3 | bdrv_attach_child_common_abort() restores the parent's AioContext. To |
8 | care to move nodes to make sure they use the same AioContext and which | 4 | do so, the child (which was supposed to be attached, but is now detached |
9 | tries both directions. | 5 | again by this abort handler) is added to the ignore list for the |
6 | AioContext changing functions. | ||
10 | 7 | ||
11 | So the problem has a simple fix: Just delete the unnecessary extra | 8 | However, since we modify a BDS's children list in the BdrvChildClass's |
12 | bdrv_try_set_aio_context() call in external_snapshot_prepare() and | 9 | .attach and .detach handlers, the child is already effectively detached |
13 | instead assert in bdrv_append() that both nodes were indeed moved to the | 10 | from the parent by this point. We do not need to put it into the ignore |
14 | same AioContext. | 11 | list. |
15 | 12 | ||
16 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 13 | Use this opportunity to clean up the empty line structure: Keep setting |
17 | Message-Id: <20200310113831.27293-6-kwolf@redhat.com> | 14 | the ignore list, invoking the AioContext function, and freeing the |
18 | Tested-by: Peter Krempa <pkrempa@redhat.com> | 15 | ignore list in blocks separated by empty lines. |
16 | |||
17 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
18 | Reviewed-by: Kevin Wolf <kwolf@redhat.com> | ||
19 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
20 | Message-Id: <20211111120829.81329-5-hreitz@redhat.com> | ||
19 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 21 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
20 | --- | 22 | --- |
21 | block.c | 1 + | 23 | block.c | 8 +++++--- |
22 | blockdev.c | 16 ---------------- | 24 | 1 file changed, 5 insertions(+), 3 deletions(-) |
23 | 2 files changed, 1 insertion(+), 16 deletions(-) | ||
24 | 25 | ||
25 | diff --git a/block.c b/block.c | 26 | diff --git a/block.c b/block.c |
26 | index XXXXXXX..XXXXXXX 100644 | 27 | index XXXXXXX..XXXXXXX 100644 |
27 | --- a/block.c | 28 | --- a/block.c |
28 | +++ b/block.c | 29 | +++ b/block.c |
29 | @@ -XXX,XX +XXX,XX @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, | 30 | @@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque) |
30 | bdrv_ref(from); | ||
31 | |||
32 | assert(qemu_get_current_aio_context() == qemu_get_aio_context()); | ||
33 | + assert(bdrv_get_aio_context(from) == bdrv_get_aio_context(to)); | ||
34 | bdrv_drained_begin(from); | ||
35 | |||
36 | /* Put all parents into @list and calculate their cumulative permissions */ | ||
37 | diff --git a/blockdev.c b/blockdev.c | ||
38 | index XXXXXXX..XXXXXXX 100644 | ||
39 | --- a/blockdev.c | ||
40 | +++ b/blockdev.c | ||
41 | @@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common, | ||
42 | DO_UPCAST(ExternalSnapshotState, common, common); | ||
43 | TransactionAction *action = common->action; | ||
44 | AioContext *aio_context; | ||
45 | - AioContext *old_context; | ||
46 | uint64_t perm, shared; | ||
47 | - int ret; | ||
48 | |||
49 | /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar | ||
50 | * purpose but a different set of parameters */ | ||
51 | @@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common, | ||
52 | goto out; | ||
53 | } | 31 | } |
54 | 32 | ||
55 | - /* Honor bdrv_try_set_aio_context() context acquisition requirements. */ | 33 | if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) { |
56 | - old_context = bdrv_get_aio_context(state->new_bs); | 34 | - GSList *ignore = g_slist_prepend(NULL, child); |
57 | - aio_context_release(aio_context); | 35 | + GSList *ignore; |
58 | - aio_context_acquire(old_context); | 36 | |
59 | - | 37 | + /* No need to ignore `child`, because it has been detached already */ |
60 | - ret = bdrv_try_set_aio_context(state->new_bs, aio_context, errp); | 38 | + ignore = NULL; |
61 | - | 39 | child->klass->can_set_aio_ctx(child, s->old_parent_ctx, &ignore, |
62 | - aio_context_release(old_context); | 40 | &error_abort); |
63 | - aio_context_acquire(aio_context); | 41 | g_slist_free(ignore); |
64 | - | 42 | - ignore = g_slist_prepend(NULL, child); |
65 | - if (ret < 0) { | 43 | - child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore); |
66 | - goto out; | 44 | |
67 | - } | 45 | + ignore = NULL; |
68 | - | 46 | + child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore); |
69 | /* This removes our old bs and adds the new bs. This is an operation that | 47 | g_slist_free(ignore); |
70 | * can fail, so we need to do it in .prepare; undoing it for abort is | 48 | } |
71 | * always possible. */ | 49 | |
72 | -- | 50 | -- |
73 | 2.20.1 | 51 | 2.31.1 |
74 | 52 | ||
75 | 53 | diff view generated by jsdifflib |
1 | blockdev-snapshot returned an error if the overlay was already in use, | 1 | From: Hanna Reitz <hreitz@redhat.com> |
---|---|---|---|
2 | which it defined as having any BlockBackend parent. This is in fact both | ||
3 | too strict (some parents can tolerate the change of visible data caused | ||
4 | by attaching a backing file) and too loose (some non-BlockBackend | ||
5 | parents may not be happy with it). | ||
6 | 2 | ||
7 | One important use case that is prevented by the too strict check is live | 3 | bdrv_replace_child_noperm() modifies BdrvChild.bs, and can potentially |
8 | storage migration with blockdev-mirror. Here, the target node is | 4 | set it to NULL. That is dangerous, because BDS parents generally assume |
9 | usually opened without a backing file so that the active layer is | 5 | that their children's .bs pointer is never NULL. We therefore want to |
10 | mirrored while its backing chain can be copied in the background. | 6 | let bdrv_replace_child_noperm() set the corresponding BdrvChild pointer |
7 | to NULL, too. | ||
11 | 8 | ||
12 | The backing chain should be attached to the mirror target node when | 9 | This patch lays the foundation for it by passing a BdrvChild ** pointer |
13 | finalising the job, just before switching the users of the source node | 10 | to bdrv_replace_child_noperm() so that it can later use it to NULL the |
14 | to the new copy (at which point the mirror job still has a reference to | 11 | BdrvChild pointer immediately after setting BdrvChild.bs to NULL. |
15 | the node). drive-mirror did this automatically, but with blockdev-mirror | ||
16 | this is the job of the QMP client, so it needs a way to do this. | ||
17 | 12 | ||
18 | blockdev-snapshot is the obvious way, so this patch makes it work in | 13 | (We will still need to undertake some intermediate steps, though.) |
19 | this scenario. The new condition is that no parent uses CONSISTENT_READ | ||
20 | permissions. This will ensure that the operation will still be blocked | ||
21 | when the node is attached to the guest device, so blockdev-snapshot | ||
22 | remains safe. | ||
23 | 14 | ||
24 | (For the sake of completeness, x-blockdev-reopen can be used to achieve | 15 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> |
25 | the same, however it is a big hammer, performs the graph change | 16 | Message-Id: <20211111120829.81329-6-hreitz@redhat.com> |
26 | completely unchecked and is still experimental. So even with the option | 17 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
27 | of using x-blockdev-reopen, there are reasons why blockdev-snapshot | ||
28 | should be able to perform this operation.) | ||
29 | |||
30 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
31 | Message-Id: <20200310113831.27293-3-kwolf@redhat.com> | ||
32 | Reviewed-by: Peter Krempa <pkrempa@redhat.com> | ||
33 | Tested-by: Peter Krempa <pkrempa@redhat.com> | ||
34 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 18 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
35 | --- | 19 | --- |
36 | blockdev.c | 14 ++++++++------ | 20 | block.c | 23 ++++++++++++----------- |
37 | tests/qemu-iotests/085.out | 4 ++-- | 21 | 1 file changed, 12 insertions(+), 11 deletions(-) |
38 | 2 files changed, 10 insertions(+), 8 deletions(-) | ||
39 | 22 | ||
40 | diff --git a/blockdev.c b/blockdev.c | 23 | diff --git a/block.c b/block.c |
41 | index XXXXXXX..XXXXXXX 100644 | 24 | index XXXXXXX..XXXXXXX 100644 |
42 | --- a/blockdev.c | 25 | --- a/block.c |
43 | +++ b/blockdev.c | 26 | +++ b/block.c |
44 | @@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common, | 27 | @@ -XXX,XX +XXX,XX @@ static BlockDriverState *bdrv_open_inherit(const char *filename, |
45 | TransactionAction *action = common->action; | 28 | static bool bdrv_recurse_has_child(BlockDriverState *bs, |
46 | AioContext *aio_context; | 29 | BlockDriverState *child); |
47 | AioContext *old_context; | 30 | |
48 | + uint64_t perm, shared; | 31 | -static void bdrv_replace_child_noperm(BdrvChild *child, |
49 | int ret; | 32 | +static void bdrv_replace_child_noperm(BdrvChild **child, |
50 | 33 | BlockDriverState *new_bs); | |
51 | /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar | 34 | static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, |
52 | @@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common, | 35 | BdrvChild *child, |
53 | goto out; | 36 | @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_abort(void *opaque) |
37 | BlockDriverState *new_bs = s->child->bs; | ||
38 | |||
39 | /* old_bs reference is transparently moved from @s to @s->child */ | ||
40 | - bdrv_replace_child_noperm(s->child, s->old_bs); | ||
41 | + bdrv_replace_child_noperm(&s->child, s->old_bs); | ||
42 | bdrv_unref(new_bs); | ||
43 | } | ||
44 | |||
45 | @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs, | ||
46 | if (new_bs) { | ||
47 | bdrv_ref(new_bs); | ||
54 | } | 48 | } |
55 | 49 | - bdrv_replace_child_noperm(child, new_bs); | |
56 | - if (bdrv_has_blk(state->new_bs)) { | 50 | + bdrv_replace_child_noperm(&child, new_bs); |
57 | + /* | 51 | /* old_bs reference is transparently moved from @child to @s */ |
58 | + * Allow attaching a backing file to an overlay that's already in use only | 52 | } |
59 | + * if the parents don't assume that they are already seeing a valid image. | 53 | |
60 | + * (Specifically, allow it as a mirror target, which is write-only access.) | 54 | @@ -XXX,XX +XXX,XX @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm) |
61 | + */ | 55 | return permissions[qapi_perm]; |
62 | + bdrv_get_cumulative_perm(state->new_bs, &perm, &shared); | 56 | } |
63 | + if (perm & BLK_PERM_CONSISTENT_READ) { | 57 | |
64 | error_setg(errp, "The overlay is already in use"); | 58 | -static void bdrv_replace_child_noperm(BdrvChild *child, |
65 | goto out; | 59 | +static void bdrv_replace_child_noperm(BdrvChild **childp, |
60 | BlockDriverState *new_bs) | ||
61 | { | ||
62 | + BdrvChild *child = *childp; | ||
63 | BlockDriverState *old_bs = child->bs; | ||
64 | int new_bs_quiesce_counter; | ||
65 | int drain_saldo; | ||
66 | @@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque) | ||
67 | BdrvChild *child = *s->child; | ||
68 | BlockDriverState *bs = child->bs; | ||
69 | |||
70 | - bdrv_replace_child_noperm(child, NULL); | ||
71 | + bdrv_replace_child_noperm(s->child, NULL); | ||
72 | |||
73 | if (bdrv_get_aio_context(bs) != s->old_child_ctx) { | ||
74 | bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort); | ||
75 | @@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_common(BlockDriverState *child_bs, | ||
66 | } | 76 | } |
67 | 77 | ||
68 | - if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, | 78 | bdrv_ref(child_bs); |
69 | - errp)) { | 79 | - bdrv_replace_child_noperm(new_child, child_bs); |
70 | - goto out; | 80 | + bdrv_replace_child_noperm(&new_child, child_bs); |
71 | - } | 81 | |
72 | - | 82 | *child = new_child; |
73 | if (state->new_bs->backing != NULL) { | 83 | |
74 | error_setg(errp, "The overlay already has a backing image"); | 84 | @@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, |
75 | goto out; | 85 | return 0; |
76 | diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out | 86 | } |
77 | index XXXXXXX..XXXXXXX 100644 | 87 | |
78 | --- a/tests/qemu-iotests/085.out | 88 | -static void bdrv_detach_child(BdrvChild *child) |
79 | +++ b/tests/qemu-iotests/085.out | 89 | +static void bdrv_detach_child(BdrvChild **childp) |
80 | @@ -XXX,XX +XXX,XX @@ Formatting 'TEST_DIR/12-snapshot-v0.IMGFMT', fmt=IMGFMT size=134217728 backing_f | 90 | { |
81 | === Invalid command - cannot create a snapshot using a file BDS === | 91 | - BlockDriverState *old_bs = child->bs; |
82 | 92 | + BlockDriverState *old_bs = (*childp)->bs; | |
83 | { 'execute': 'blockdev-snapshot', 'arguments': { 'node':'virtio0', 'overlay':'file_12' } } | 93 | |
84 | -{"error": {"class": "GenericError", "desc": "The overlay does not support backing images"}} | 94 | - bdrv_replace_child_noperm(child, NULL); |
85 | +{"error": {"class": "GenericError", "desc": "The overlay is already in use"}} | 95 | - bdrv_child_free(child); |
86 | 96 | + bdrv_replace_child_noperm(childp, NULL); | |
87 | === Invalid command - snapshot node used as active layer === | 97 | + bdrv_child_free(*childp); |
88 | 98 | ||
89 | @@ -XXX,XX +XXX,XX @@ Formatting 'TEST_DIR/12-snapshot-v0.IMGFMT', fmt=IMGFMT size=134217728 backing_f | 99 | if (old_bs) { |
90 | === Invalid command - snapshot node used as backing hd === | 100 | /* |
91 | 101 | @@ -XXX,XX +XXX,XX @@ void bdrv_root_unref_child(BdrvChild *child) | |
92 | { 'execute': 'blockdev-snapshot', 'arguments': { 'node': 'virtio0', 'overlay':'snap_11' } } | 102 | BlockDriverState *child_bs; |
93 | -{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node is used as backing hd of 'snap_12'"}} | 103 | |
94 | +{"error": {"class": "GenericError", "desc": "The overlay is already in use"}} | 104 | child_bs = child->bs; |
95 | 105 | - bdrv_detach_child(child); | |
96 | === Invalid command - snapshot node has a backing image === | 106 | + bdrv_detach_child(&child); |
107 | bdrv_unref(child_bs); | ||
108 | } | ||
97 | 109 | ||
98 | -- | 110 | -- |
99 | 2.20.1 | 111 | 2.31.1 |
100 | 112 | ||
101 | 113 | diff view generated by jsdifflib |
1 | From: Daniel Henrique Barboza <danielhb413@gmail.com> | 1 | From: Hanna Reitz <hreitz@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | When using a non-UTF8 secret to create a volume using qemu-img, the | 3 | As of a future patch, bdrv_replace_child_tran() will take a BdrvChild ** |
4 | following error happens: | 4 | pointer. Prepare for that by getting such a pointer and using it where |
5 | applicable, and (dereferenced) as a parameter for | ||
6 | bdrv_replace_child_tran(). | ||
5 | 7 | ||
6 | $ qemu-img create -f luks --object secret,id=vol_1_encrypt0,file=vol_resize_pool.vol_1.secret.qzVQrI -o key-secret=vol_1_encrypt0 /var/tmp/pool_target/vol_1 10240K | 8 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> |
7 | 9 | Message-Id: <20211111120829.81329-7-hreitz@redhat.com> | |
8 | Formatting '/var/tmp/pool_target/vol_1', fmt=luks size=10485760 key-secret=vol_1_encrypt0 | 10 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
9 | qemu-img: /var/tmp/pool_target/vol_1: Data from secret vol_1_encrypt0 is not valid UTF-8 | ||
10 | |||
11 | However, the created file '/var/tmp/pool_target/vol_1' is left behind in the | ||
12 | file system after the failure. This behavior can be observed when creating | ||
13 | the volume using Libvirt, via 'virsh vol-create', and then getting "volume | ||
14 | target path already exist" errors when trying to re-create the volume. | ||
15 | |||
16 | The volume file is created inside block_crypto_co_create_opts_luks(), in | ||
17 | block/crypto.c. If the bdrv_create_file() call is successful but any | ||
18 | succeeding step fails*, the existing 'fail' label does not take into | ||
19 | account the created file, leaving it behind. | ||
20 | |||
21 | This patch changes block_crypto_co_create_opts_luks() to delete | ||
22 | 'filename' in case of failure. A failure in this point means that | ||
23 | the volume is now truncated/corrupted, so even if 'filename' was an | ||
24 | existing volume before calling qemu-img, it is now unusable. Deleting | ||
25 | the file it is not much worse than leaving it in the filesystem in | ||
26 | this scenario, and we don't have to deal with checking the file | ||
27 | pre-existence in the code. | ||
28 | |||
29 | * in our case, block_crypto_co_create_generic calls qcrypto_block_create, | ||
30 | which calls qcrypto_block_luks_create, and this function fails when | ||
31 | calling qcrypto_secret_lookup_as_utf8. | ||
32 | |||
33 | Reported-by: Srikanth Aithal <bssrikanth@in.ibm.com> | ||
34 | Suggested-by: Kevin Wolf <kwolf@redhat.com> | ||
35 | Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> | ||
36 | Message-Id: <20200130213907.2830642-4-danielhb413@gmail.com> | ||
37 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 11 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
38 | --- | 12 | --- |
39 | block/crypto.c | 18 ++++++++++++++++++ | 13 | block.c | 21 ++++++++++++--------- |
40 | 1 file changed, 18 insertions(+) | 14 | 1 file changed, 12 insertions(+), 9 deletions(-) |
41 | 15 | ||
42 | diff --git a/block/crypto.c b/block/crypto.c | 16 | diff --git a/block.c b/block.c |
43 | index XXXXXXX..XXXXXXX 100644 | 17 | index XXXXXXX..XXXXXXX 100644 |
44 | --- a/block/crypto.c | 18 | --- a/block.c |
45 | +++ b/block/crypto.c | 19 | +++ b/block.c |
46 | @@ -XXX,XX +XXX,XX @@ | 20 | @@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, |
47 | #include "qapi/error.h" | 21 | BdrvChild *child, |
48 | #include "qemu/module.h" | 22 | Transaction *tran) |
49 | #include "qemu/option.h" | 23 | { |
50 | +#include "qemu/cutils.h" | 24 | + BdrvChild **childp; |
51 | #include "crypto.h" | 25 | BdrvRemoveFilterOrCowChild *s; |
52 | 26 | ||
53 | typedef struct BlockCrypto BlockCrypto; | 27 | - assert(child == bs->backing || child == bs->file); |
54 | @@ -XXX,XX +XXX,XX @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, | 28 | - |
55 | 29 | if (!child) { | |
56 | ret = 0; | 30 | return; |
57 | fail: | 31 | } |
58 | + /* | 32 | |
59 | + * If an error occurred, delete 'filename'. Even if the file existed | 33 | + if (child == bs->backing) { |
60 | + * beforehand, it has been truncated and corrupted in the process. | 34 | + childp = &bs->backing; |
61 | + */ | 35 | + } else if (child == bs->file) { |
62 | + if (ret && bs) { | 36 | + childp = &bs->file; |
63 | + Error *local_delete_err = NULL; | 37 | + } else { |
64 | + int r_del = bdrv_co_delete_file(bs, &local_delete_err); | 38 | + g_assert_not_reached(); |
65 | + /* | ||
66 | + * ENOTSUP will happen if the block driver doesn't support | ||
67 | + * the 'bdrv_co_delete_file' interface. This is a predictable | ||
68 | + * scenario and shouldn't be reported back to the user. | ||
69 | + */ | ||
70 | + if ((r_del < 0) && (r_del != -ENOTSUP)) { | ||
71 | + error_report_err(local_delete_err); | ||
72 | + } | ||
73 | + } | 39 | + } |
74 | + | 40 | + |
75 | bdrv_unref(bs); | 41 | if (child->bs) { |
76 | qapi_free_QCryptoBlockCreateOptions(create_opts); | 42 | - bdrv_replace_child_tran(child, NULL, tran); |
77 | qobject_unref(cryptoopts); | 43 | + bdrv_replace_child_tran(*childp, NULL, tran); |
44 | } | ||
45 | |||
46 | s = g_new(BdrvRemoveFilterOrCowChild, 1); | ||
47 | *s = (BdrvRemoveFilterOrCowChild) { | ||
48 | .child = child, | ||
49 | - .is_backing = (child == bs->backing), | ||
50 | + .is_backing = (childp == &bs->backing), | ||
51 | }; | ||
52 | tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s); | ||
53 | |||
54 | - if (s->is_backing) { | ||
55 | - bs->backing = NULL; | ||
56 | - } else { | ||
57 | - bs->file = NULL; | ||
58 | - } | ||
59 | + *childp = NULL; | ||
60 | } | ||
61 | |||
62 | /* | ||
78 | -- | 63 | -- |
79 | 2.20.1 | 64 | 2.31.1 |
80 | 65 | ||
81 | 66 | diff view generated by jsdifflib |
1 | From: Peter Krempa <pkrempa@redhat.com> | 1 | From: Hanna Reitz <hreitz@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | Anounce that 'blockdev-snapshot' command's permissions allow changing | 3 | Invoke the transaction drivers' .clean() methods only after all |
4 | of the backing file if the 'consistent_read' permission is not required. | 4 | .commit() or .abort() handlers are done. |
5 | 5 | ||
6 | This is useful for libvirt to allow late opening of the backing chain | 6 | This makes it easier to have nested transactions where the top-level |
7 | during a blockdev-mirror. | 7 | transactions pass objects to lower transactions that the latter can |
8 | still use throughout their commit/abort phases, while the top-level | ||
9 | transaction keeps a reference that is released in its .clean() method. | ||
8 | 10 | ||
9 | Signed-off-by: Peter Krempa <pkrempa@redhat.com> | 11 | (Before this commit, that is also possible, but the top-level |
10 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 12 | transaction would need to take care to invoke tran_add() before the |
11 | Message-Id: <20200310113831.27293-8-kwolf@redhat.com> | 13 | lower-level transaction does. This commit makes the ordering |
14 | irrelevant, which is just a bit nicer.) | ||
15 | |||
16 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
17 | Message-Id: <20211111120829.81329-8-hreitz@redhat.com> | ||
18 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
12 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 19 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
13 | --- | 20 | --- |
14 | qapi/block-core.json | 9 ++++++++- | 21 | include/qemu/transactions.h | 3 +++ |
15 | 1 file changed, 8 insertions(+), 1 deletion(-) | 22 | util/transactions.c | 8 ++++++-- |
23 | 2 files changed, 9 insertions(+), 2 deletions(-) | ||
16 | 24 | ||
17 | diff --git a/qapi/block-core.json b/qapi/block-core.json | 25 | diff --git a/include/qemu/transactions.h b/include/qemu/transactions.h |
18 | index XXXXXXX..XXXXXXX 100644 | 26 | index XXXXXXX..XXXXXXX 100644 |
19 | --- a/qapi/block-core.json | 27 | --- a/include/qemu/transactions.h |
20 | +++ b/qapi/block-core.json | 28 | +++ b/include/qemu/transactions.h |
21 | @@ -XXX,XX +XXX,XX @@ | 29 | @@ -XXX,XX +XXX,XX @@ |
22 | # | 30 | * tran_create(), call your "prepare" functions on it, and finally call |
23 | # For the arguments, see the documentation of BlockdevSnapshot. | 31 | * tran_abort() or tran_commit() to finalize the transaction by corresponding |
24 | # | 32 | * finalization actions in reverse order. |
25 | +# Features: | 33 | + * |
26 | +# @allow-write-only-overlay: If present, the check whether this operation is safe | 34 | + * The clean() functions registered by the drivers in a transaction are called |
27 | +# was relaxed so that it can be used to change | 35 | + * last, after all abort() or commit() functions have been called. |
28 | +# backing file of a destination of a blockdev-mirror. | 36 | */ |
29 | +# (since 5.0) | 37 | |
30 | +# | 38 | #ifndef QEMU_TRANSACTIONS_H |
31 | # Since: 2.5 | 39 | diff --git a/util/transactions.c b/util/transactions.c |
32 | # | 40 | index XXXXXXX..XXXXXXX 100644 |
33 | # Example: | 41 | --- a/util/transactions.c |
34 | @@ -XXX,XX +XXX,XX @@ | 42 | +++ b/util/transactions.c |
35 | # | 43 | @@ -XXX,XX +XXX,XX @@ void tran_abort(Transaction *tran) |
36 | ## | 44 | { |
37 | { 'command': 'blockdev-snapshot', | 45 | TransactionAction *act, *next; |
38 | - 'data': 'BlockdevSnapshot' } | 46 | |
39 | + 'data': 'BlockdevSnapshot', | 47 | - QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) { |
40 | + 'features': [ 'allow-write-only-overlay' ] } | 48 | + QSLIST_FOREACH(act, &tran->actions, entry) { |
41 | 49 | if (act->drv->abort) { | |
42 | ## | 50 | act->drv->abort(act->opaque); |
43 | # @change-backing-file: | 51 | } |
52 | + } | ||
53 | |||
54 | + QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) { | ||
55 | if (act->drv->clean) { | ||
56 | act->drv->clean(act->opaque); | ||
57 | } | ||
58 | @@ -XXX,XX +XXX,XX @@ void tran_commit(Transaction *tran) | ||
59 | { | ||
60 | TransactionAction *act, *next; | ||
61 | |||
62 | - QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) { | ||
63 | + QSLIST_FOREACH(act, &tran->actions, entry) { | ||
64 | if (act->drv->commit) { | ||
65 | act->drv->commit(act->opaque); | ||
66 | } | ||
67 | + } | ||
68 | |||
69 | + QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) { | ||
70 | if (act->drv->clean) { | ||
71 | act->drv->clean(act->opaque); | ||
72 | } | ||
44 | -- | 73 | -- |
45 | 2.20.1 | 74 | 2.31.1 |
46 | 75 | ||
47 | 76 | diff view generated by jsdifflib |
1 | From: Daniel Henrique Barboza <danielhb413@gmail.com> | 1 | From: Hanna Reitz <hreitz@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | Using the new 'bdrv_co_delete_file' interface, a pure co_routine function | 3 | As of a future commit, bdrv_replace_child_noperm() will clear the |
4 | 'bdrv_co_delete_file' inside block.c can can be used in a way similar of | 4 | indirect BdrvChild pointer passed to it if the new child BDS is NULL. |
5 | the existing bdrv_create_file to to clean up a created file. | 5 | bdrv_replace_child_tran() will want to let it do that, but revert this |
6 | 6 | change in its abort handler. For that, we need to have it receive a | |
7 | We're creating a pure co_routine because the only caller of | 7 | BdrvChild ** pointer, too, and keep it stored in the |
8 | 'bdrv_co_delete_file' will be already in co_routine context, thus there | 8 | BdrvReplaceChildState object that we attach to the transaction. |
9 | is no need to add all the machinery to check for qemu_in_coroutine() and | 9 | |
10 | create a separated co_routine to do the job. | 10 | Note that we do not need to store it in the BdrvReplaceChildState when |
11 | 11 | new_bs is not NULL, because then there is nothing to revert. This is | |
12 | Suggested-by: Daniel P. Berrangé <berrange@redhat.com> | 12 | important so that bdrv_replace_node_noperm() can pass a pointer to a |
13 | Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> | 13 | loop-local variable to bdrv_replace_child_tran() without worrying that |
14 | Message-Id: <20200130213907.2830642-3-danielhb413@gmail.com> | 14 | this pointer will outlive one loop iteration. |
15 | |||
16 | (Of course, for that to work, bdrv_replace_node_noperm() and in turn | ||
17 | bdrv_replace_node() and its relatives may not be called with a NULL @to | ||
18 | node. Luckily, they already are not, but now we should assert this.) | ||
19 | |||
20 | bdrv_remove_file_or_backing_child() on the other hand needs to ensure | ||
21 | that the indirect pointer it passes will stay valid for the duration of | ||
22 | the transaction. Ensure this by keeping a strong reference to the BDS | ||
23 | whose &bs->backing or &bs->file it passes to bdrv_replace_child_tran(), | ||
24 | and giving up that reference only in the transaction .clean() handler. | ||
25 | |||
26 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
27 | Message-Id: <20211111120829.81329-9-hreitz@redhat.com> | ||
28 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
15 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 29 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
16 | --- | 30 | --- |
17 | include/block/block.h | 1 + | 31 | block.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++------- |
18 | block.c | 26 ++++++++++++++++++++++++++ | 32 | 1 file changed, 73 insertions(+), 10 deletions(-) |
19 | 2 files changed, 27 insertions(+) | 33 | |
20 | |||
21 | diff --git a/include/block/block.h b/include/block/block.h | ||
22 | index XXXXXXX..XXXXXXX 100644 | ||
23 | --- a/include/block/block.h | ||
24 | +++ b/include/block/block.h | ||
25 | @@ -XXX,XX +XXX,XX @@ bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base, | ||
26 | int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, | ||
27 | Error **errp); | ||
28 | void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base); | ||
29 | +int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp); | ||
30 | |||
31 | |||
32 | typedef struct BdrvCheckResult { | ||
33 | diff --git a/block.c b/block.c | 34 | diff --git a/block.c b/block.c |
34 | index XXXXXXX..XXXXXXX 100644 | 35 | index XXXXXXX..XXXXXXX 100644 |
35 | --- a/block.c | 36 | --- a/block.c |
36 | +++ b/block.c | 37 | +++ b/block.c |
37 | @@ -XXX,XX +XXX,XX @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp) | 38 | @@ -XXX,XX +XXX,XX @@ static int bdrv_drv_set_perm(BlockDriverState *bs, uint64_t perm, |
38 | } | 39 | |
39 | } | 40 | typedef struct BdrvReplaceChildState { |
40 | 41 | BdrvChild *child; | |
41 | +int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp) | 42 | + BdrvChild **childp; |
43 | BlockDriverState *old_bs; | ||
44 | } BdrvReplaceChildState; | ||
45 | |||
46 | @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_abort(void *opaque) | ||
47 | BdrvReplaceChildState *s = opaque; | ||
48 | BlockDriverState *new_bs = s->child->bs; | ||
49 | |||
50 | - /* old_bs reference is transparently moved from @s to @s->child */ | ||
51 | + /* | ||
52 | + * old_bs reference is transparently moved from @s to s->child. | ||
53 | + * | ||
54 | + * Pass &s->child here instead of s->childp, because: | ||
55 | + * (1) s->old_bs must be non-NULL, so bdrv_replace_child_noperm() will not | ||
56 | + * modify the BdrvChild * pointer we indirectly pass to it, i.e. it | ||
57 | + * will not modify s->child. From that perspective, it does not matter | ||
58 | + * whether we pass s->childp or &s->child. | ||
59 | + * (TODO: Right now, bdrv_replace_child_noperm() never modifies that | ||
60 | + * pointer anyway (though it will in the future), so at this point it | ||
61 | + * absolutely does not matter whether we pass s->childp or &s->child.) | ||
62 | + * (2) If new_bs is not NULL, s->childp will be NULL. We then cannot use | ||
63 | + * it here. | ||
64 | + * (3) If new_bs is NULL, *s->childp will have been NULLed by | ||
65 | + * bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we | ||
66 | + * must not pass a NULL *s->childp here. | ||
67 | + * (TODO: In its current state, bdrv_replace_child_noperm() will not | ||
68 | + * have NULLed *s->childp, so this does not apply yet. It will in the | ||
69 | + * future.) | ||
70 | + * | ||
71 | + * So whether new_bs was NULL or not, we cannot pass s->childp here; and in | ||
72 | + * any case, there is no reason to pass it anyway. | ||
73 | + */ | ||
74 | bdrv_replace_child_noperm(&s->child, s->old_bs); | ||
75 | bdrv_unref(new_bs); | ||
76 | } | ||
77 | @@ -XXX,XX +XXX,XX @@ static TransactionActionDrv bdrv_replace_child_drv = { | ||
78 | * Note: real unref of old_bs is done only on commit. | ||
79 | * | ||
80 | * The function doesn't update permissions, caller is responsible for this. | ||
81 | + * | ||
82 | + * Note that if new_bs == NULL, @childp is stored in a state object attached | ||
83 | + * to @tran, so that the old child can be reinstated in the abort handler. | ||
84 | + * Therefore, if @new_bs can be NULL, @childp must stay valid until the | ||
85 | + * transaction is committed or aborted. | ||
86 | + * | ||
87 | + * (TODO: The reinstating does not happen yet, but it will once | ||
88 | + * bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.) | ||
89 | */ | ||
90 | -static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs, | ||
91 | +static void bdrv_replace_child_tran(BdrvChild **childp, | ||
92 | + BlockDriverState *new_bs, | ||
93 | Transaction *tran) | ||
94 | { | ||
95 | BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1); | ||
96 | *s = (BdrvReplaceChildState) { | ||
97 | - .child = child, | ||
98 | - .old_bs = child->bs, | ||
99 | + .child = *childp, | ||
100 | + .childp = new_bs == NULL ? childp : NULL, | ||
101 | + .old_bs = (*childp)->bs, | ||
102 | }; | ||
103 | tran_add(tran, &bdrv_replace_child_drv, s); | ||
104 | |||
105 | if (new_bs) { | ||
106 | bdrv_ref(new_bs); | ||
107 | } | ||
108 | - bdrv_replace_child_noperm(&child, new_bs); | ||
109 | - /* old_bs reference is transparently moved from @child to @s */ | ||
110 | + bdrv_replace_child_noperm(childp, new_bs); | ||
111 | + /* old_bs reference is transparently moved from *childp to @s */ | ||
112 | } | ||
113 | |||
114 | /* | ||
115 | @@ -XXX,XX +XXX,XX @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to) | ||
116 | |||
117 | typedef struct BdrvRemoveFilterOrCowChild { | ||
118 | BdrvChild *child; | ||
119 | + BlockDriverState *bs; | ||
120 | bool is_backing; | ||
121 | } BdrvRemoveFilterOrCowChild; | ||
122 | |||
123 | @@ -XXX,XX +XXX,XX @@ static void bdrv_remove_filter_or_cow_child_commit(void *opaque) | ||
124 | bdrv_child_free(s->child); | ||
125 | } | ||
126 | |||
127 | +static void bdrv_remove_filter_or_cow_child_clean(void *opaque) | ||
42 | +{ | 128 | +{ |
43 | + Error *local_err = NULL; | 129 | + BdrvRemoveFilterOrCowChild *s = opaque; |
44 | + int ret; | 130 | + |
45 | + | 131 | + /* Drop the bs reference after the transaction is done */ |
46 | + assert(bs != NULL); | 132 | + bdrv_unref(s->bs); |
47 | + | 133 | + g_free(s); |
48 | + if (!bs->drv) { | ||
49 | + error_setg(errp, "Block node '%s' is not opened", bs->filename); | ||
50 | + return -ENOMEDIUM; | ||
51 | + } | ||
52 | + | ||
53 | + if (!bs->drv->bdrv_co_delete_file) { | ||
54 | + error_setg(errp, "Driver '%s' does not support image deletion", | ||
55 | + bs->drv->format_name); | ||
56 | + return -ENOTSUP; | ||
57 | + } | ||
58 | + | ||
59 | + ret = bs->drv->bdrv_co_delete_file(bs, &local_err); | ||
60 | + if (ret < 0) { | ||
61 | + error_propagate(errp, local_err); | ||
62 | + } | ||
63 | + | ||
64 | + return ret; | ||
65 | +} | 134 | +} |
66 | + | 135 | + |
67 | /** | 136 | static TransactionActionDrv bdrv_remove_filter_or_cow_child_drv = { |
68 | * Try to get @bs's logical and physical block size. | 137 | .abort = bdrv_remove_filter_or_cow_child_abort, |
69 | * On success, store them in @bsz struct and return 0. | 138 | .commit = bdrv_remove_filter_or_cow_child_commit, |
139 | - .clean = g_free, | ||
140 | + .clean = bdrv_remove_filter_or_cow_child_clean, | ||
141 | }; | ||
142 | |||
143 | /* | ||
144 | @@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, | ||
145 | return; | ||
146 | } | ||
147 | |||
148 | + /* | ||
149 | + * Keep a reference to @bs so @childp will stay valid throughout the | ||
150 | + * transaction (required by bdrv_replace_child_tran()) | ||
151 | + */ | ||
152 | + bdrv_ref(bs); | ||
153 | if (child == bs->backing) { | ||
154 | childp = &bs->backing; | ||
155 | } else if (child == bs->file) { | ||
156 | @@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, | ||
157 | } | ||
158 | |||
159 | if (child->bs) { | ||
160 | - bdrv_replace_child_tran(*childp, NULL, tran); | ||
161 | + bdrv_replace_child_tran(childp, NULL, tran); | ||
162 | } | ||
163 | |||
164 | s = g_new(BdrvRemoveFilterOrCowChild, 1); | ||
165 | *s = (BdrvRemoveFilterOrCowChild) { | ||
166 | .child = child, | ||
167 | + .bs = bs, | ||
168 | .is_backing = (childp == &bs->backing), | ||
169 | }; | ||
170 | tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s); | ||
171 | @@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from, | ||
172 | { | ||
173 | BdrvChild *c, *next; | ||
174 | |||
175 | + assert(to != NULL); | ||
176 | + | ||
177 | QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) { | ||
178 | assert(c->bs == from); | ||
179 | if (!should_update_child(c, to)) { | ||
180 | @@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from, | ||
181 | c->name, from->node_name); | ||
182 | return -EPERM; | ||
183 | } | ||
184 | - bdrv_replace_child_tran(c, to, tran); | ||
185 | + | ||
186 | + /* | ||
187 | + * Passing a pointer to the local variable @c is fine here, because | ||
188 | + * @to is not NULL, and so &c will not be attached to the transaction. | ||
189 | + */ | ||
190 | + bdrv_replace_child_tran(&c, to, tran); | ||
191 | } | ||
192 | |||
193 | return 0; | ||
194 | @@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from, | ||
195 | * | ||
196 | * With @detach_subchain=true @to must be in a backing chain of @from. In this | ||
197 | * case backing link of the cow-parent of @to is removed. | ||
198 | + * | ||
199 | + * @to must not be NULL. | ||
200 | */ | ||
201 | static int bdrv_replace_node_common(BlockDriverState *from, | ||
202 | BlockDriverState *to, | ||
203 | @@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_common(BlockDriverState *from, | ||
204 | BlockDriverState *to_cow_parent = NULL; | ||
205 | int ret; | ||
206 | |||
207 | + assert(to != NULL); | ||
208 | + | ||
209 | if (detach_subchain) { | ||
210 | assert(bdrv_chain_contains(from, to)); | ||
211 | assert(from != to); | ||
212 | @@ -XXX,XX +XXX,XX @@ out: | ||
213 | return ret; | ||
214 | } | ||
215 | |||
216 | +/** | ||
217 | + * Replace node @from by @to (where neither may be NULL). | ||
218 | + */ | ||
219 | int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, | ||
220 | Error **errp) | ||
221 | { | ||
222 | @@ -XXX,XX +XXX,XX @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs, | ||
223 | bdrv_drained_begin(old_bs); | ||
224 | bdrv_drained_begin(new_bs); | ||
225 | |||
226 | - bdrv_replace_child_tran(child, new_bs, tran); | ||
227 | + bdrv_replace_child_tran(&child, new_bs, tran); | ||
228 | |||
229 | found = g_hash_table_new(NULL, NULL); | ||
230 | refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs); | ||
70 | -- | 231 | -- |
71 | 2.20.1 | 232 | 2.31.1 |
72 | 233 | ||
73 | 234 | diff view generated by jsdifflib |
1 | The newly tested scenario is a common live storage migration scenario: | 1 | From: Hanna Reitz <hreitz@redhat.com> |
---|---|---|---|
2 | The target node is opened without a backing file so that the active | 2 | |
3 | layer is mirrored while its backing chain can be copied in the | 3 | In most of the block layer, especially when traversing down from other |
4 | background. | 4 | BlockDriverStates, we assume that BdrvChild.bs can never be NULL. When |
5 | 5 | it becomes NULL, it is expected that the corresponding BdrvChild pointer | |
6 | The backing chain should be attached to the mirror target node when | 6 | also becomes NULL and the BdrvChild object is freed. |
7 | finalising the job, just before switching the users of the source node | 7 | |
8 | to the new copy (at which point the mirror job still has a reference to | 8 | Therefore, once bdrv_replace_child_noperm() sets the BdrvChild.bs |
9 | the node). drive-mirror did this automatically, but with blockdev-mirror | 9 | pointer to NULL, it should also immediately set the corresponding |
10 | this is the job of the QMP client. | 10 | BdrvChild pointer (like bs->file or bs->backing) to NULL. |
11 | 11 | ||
12 | This patch adds test cases for two ways to achieve the desired result, | 12 | In that context, it also makes sense for this function to free the |
13 | using either x-blockdev-reopen or blockdev-snapshot. | 13 | child. Sometimes we cannot do so, though, because it is called in a |
14 | 14 | transactional context where the caller might still want to reinstate the | |
15 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 15 | child in the abort branch (and free it only on commit), so this behavior |
16 | Message-Id: <20200310113831.27293-5-kwolf@redhat.com> | 16 | has to remain optional. |
17 | Reviewed-by: Peter Krempa <pkrempa@redhat.com> | 17 | |
18 | In bdrv_replace_child_tran()'s abort handler, we now rely on the fact | ||
19 | that the BdrvChild passed to bdrv_replace_child_tran() must have had a | ||
20 | non-NULL .bs pointer initially. Make a note of that and assert it. | ||
21 | |||
22 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
23 | Message-Id: <20211111120829.81329-10-hreitz@redhat.com> | ||
18 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 24 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
19 | --- | 25 | --- |
20 | tests/qemu-iotests/155 | 56 ++++++++++++++++++++++++++++++++++---- | 26 | block.c | 102 +++++++++++++++++++++++++++++++++++++++++++------------- |
21 | tests/qemu-iotests/155.out | 4 +-- | 27 | 1 file changed, 79 insertions(+), 23 deletions(-) |
22 | 2 files changed, 53 insertions(+), 7 deletions(-) | 28 | |
23 | 29 | diff --git a/block.c b/block.c | |
24 | diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155 | 30 | index XXXXXXX..XXXXXXX 100644 |
25 | index XXXXXXX..XXXXXXX 100755 | 31 | --- a/block.c |
26 | --- a/tests/qemu-iotests/155 | 32 | +++ b/block.c |
27 | +++ b/tests/qemu-iotests/155 | 33 | @@ -XXX,XX +XXX,XX @@ static BlockDriverState *bdrv_open_inherit(const char *filename, |
28 | @@ -XXX,XX +XXX,XX @@ target_img = os.path.join(iotests.test_dir, 'target.' + iotests.imgfmt) | 34 | static bool bdrv_recurse_has_child(BlockDriverState *bs, |
29 | # image during runtime, only makes sense if | 35 | BlockDriverState *child); |
30 | # target_blockdev_backing is not None | 36 | |
31 | # (None: same as target_backing) | 37 | +static void bdrv_child_free(BdrvChild *child); |
32 | +# target_open_with_backing: If True, the target image is added with its backing | 38 | static void bdrv_replace_child_noperm(BdrvChild **child, |
33 | +# chain opened right away. If False, blockdev-add | 39 | - BlockDriverState *new_bs); |
34 | +# opens it without a backing file and job completion | 40 | + BlockDriverState *new_bs, |
35 | +# is supposed to open the backing chain. | 41 | + bool free_empty_child); |
36 | 42 | static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, | |
37 | class BaseClass(iotests.QMPTestCase): | 43 | BdrvChild *child, |
38 | target_blockdev_backing = None | 44 | Transaction *tran); |
39 | target_real_backing = None | 45 | @@ -XXX,XX +XXX,XX @@ typedef struct BdrvReplaceChildState { |
40 | + target_open_with_backing = True | 46 | BdrvChild *child; |
41 | 47 | BdrvChild **childp; | |
42 | def setUp(self): | 48 | BlockDriverState *old_bs; |
43 | qemu_img('create', '-f', iotests.imgfmt, back0_img, '1440K') | 49 | + bool free_empty_child; |
44 | @@ -XXX,XX +XXX,XX @@ class BaseClass(iotests.QMPTestCase): | 50 | } BdrvReplaceChildState; |
45 | options = { 'node-name': 'target', | 51 | |
46 | 'driver': iotests.imgfmt, | 52 | static void bdrv_replace_child_commit(void *opaque) |
47 | 'file': { 'driver': 'file', | 53 | { |
48 | + 'node-name': 'target-file', | 54 | BdrvReplaceChildState *s = opaque; |
49 | 'filename': target_img } } | 55 | |
50 | - if self.target_blockdev_backing: | 56 | + if (s->free_empty_child && !s->child->bs) { |
51 | - options['backing'] = self.target_blockdev_backing | 57 | + bdrv_child_free(s->child); |
58 | + } | ||
59 | bdrv_unref(s->old_bs); | ||
60 | } | ||
61 | |||
62 | @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_abort(void *opaque) | ||
63 | * modify the BdrvChild * pointer we indirectly pass to it, i.e. it | ||
64 | * will not modify s->child. From that perspective, it does not matter | ||
65 | * whether we pass s->childp or &s->child. | ||
66 | - * (TODO: Right now, bdrv_replace_child_noperm() never modifies that | ||
67 | - * pointer anyway (though it will in the future), so at this point it | ||
68 | - * absolutely does not matter whether we pass s->childp or &s->child.) | ||
69 | * (2) If new_bs is not NULL, s->childp will be NULL. We then cannot use | ||
70 | * it here. | ||
71 | * (3) If new_bs is NULL, *s->childp will have been NULLed by | ||
72 | * bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we | ||
73 | * must not pass a NULL *s->childp here. | ||
74 | - * (TODO: In its current state, bdrv_replace_child_noperm() will not | ||
75 | - * have NULLed *s->childp, so this does not apply yet. It will in the | ||
76 | - * future.) | ||
77 | * | ||
78 | * So whether new_bs was NULL or not, we cannot pass s->childp here; and in | ||
79 | * any case, there is no reason to pass it anyway. | ||
80 | */ | ||
81 | - bdrv_replace_child_noperm(&s->child, s->old_bs); | ||
82 | + bdrv_replace_child_noperm(&s->child, s->old_bs, true); | ||
83 | + /* | ||
84 | + * The child was pre-existing, so s->old_bs must be non-NULL, and | ||
85 | + * s->child thus must not have been freed | ||
86 | + */ | ||
87 | + assert(s->child != NULL); | ||
88 | + if (!new_bs) { | ||
89 | + /* As described above, *s->childp was cleared, so restore it */ | ||
90 | + assert(s->childp != NULL); | ||
91 | + *s->childp = s->child; | ||
92 | + } | ||
93 | bdrv_unref(new_bs); | ||
94 | } | ||
95 | |||
96 | @@ -XXX,XX +XXX,XX @@ static TransactionActionDrv bdrv_replace_child_drv = { | ||
97 | * | ||
98 | * The function doesn't update permissions, caller is responsible for this. | ||
99 | * | ||
100 | + * (*childp)->bs must not be NULL. | ||
101 | + * | ||
102 | * Note that if new_bs == NULL, @childp is stored in a state object attached | ||
103 | * to @tran, so that the old child can be reinstated in the abort handler. | ||
104 | * Therefore, if @new_bs can be NULL, @childp must stay valid until the | ||
105 | * transaction is committed or aborted. | ||
106 | * | ||
107 | - * (TODO: The reinstating does not happen yet, but it will once | ||
108 | - * bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.) | ||
109 | + * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is | ||
110 | + * freed (on commit). @free_empty_child should only be false if the | ||
111 | + * caller will free the BDrvChild themselves (which may be important | ||
112 | + * if this is in turn called in another transactional context). | ||
113 | */ | ||
114 | static void bdrv_replace_child_tran(BdrvChild **childp, | ||
115 | BlockDriverState *new_bs, | ||
116 | - Transaction *tran) | ||
117 | + Transaction *tran, | ||
118 | + bool free_empty_child) | ||
119 | { | ||
120 | BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1); | ||
121 | *s = (BdrvReplaceChildState) { | ||
122 | .child = *childp, | ||
123 | .childp = new_bs == NULL ? childp : NULL, | ||
124 | .old_bs = (*childp)->bs, | ||
125 | + .free_empty_child = free_empty_child, | ||
126 | }; | ||
127 | tran_add(tran, &bdrv_replace_child_drv, s); | ||
128 | |||
129 | + /* The abort handler relies on this */ | ||
130 | + assert(s->old_bs != NULL); | ||
52 | + | 131 | + |
53 | + if not self.target_open_with_backing: | 132 | if (new_bs) { |
54 | + options['backing'] = None | 133 | bdrv_ref(new_bs); |
55 | + elif self.target_blockdev_backing: | 134 | } |
56 | + options['backing'] = self.target_blockdev_backing | 135 | - bdrv_replace_child_noperm(childp, new_bs); |
57 | 136 | + /* | |
58 | result = self.vm.qmp('blockdev-add', **options) | 137 | + * Pass free_empty_child=false, we will free the child (if |
59 | self.assert_qmp(result, 'return', {}) | 138 | + * necessary) in bdrv_replace_child_commit() (if our |
60 | @@ -XXX,XX +XXX,XX @@ class BaseClass(iotests.QMPTestCase): | 139 | + * @free_empty_child parameter was true). |
61 | # cmd: Mirroring command to execute, either drive-mirror or blockdev-mirror | 140 | + */ |
62 | 141 | + bdrv_replace_child_noperm(childp, new_bs, false); | |
63 | class MirrorBaseClass(BaseClass): | 142 | /* old_bs reference is transparently moved from *childp to @s */ |
64 | + def openBacking(self): | 143 | } |
65 | + pass | 144 | |
145 | @@ -XXX,XX +XXX,XX @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm) | ||
146 | return permissions[qapi_perm]; | ||
147 | } | ||
148 | |||
149 | +/** | ||
150 | + * Replace (*childp)->bs by @new_bs. | ||
151 | + * | ||
152 | + * If @new_bs is NULL, *childp will be set to NULL, too: BDS parents | ||
153 | + * generally cannot handle a BdrvChild with .bs == NULL, so clearing | ||
154 | + * BdrvChild.bs should generally immediately be followed by the | ||
155 | + * BdrvChild pointer being cleared as well. | ||
156 | + * | ||
157 | + * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is | ||
158 | + * freed. @free_empty_child should only be false if the caller will | ||
159 | + * free the BdrvChild themselves (this may be important in a | ||
160 | + * transactional context, where it may only be freed on commit). | ||
161 | + */ | ||
162 | static void bdrv_replace_child_noperm(BdrvChild **childp, | ||
163 | - BlockDriverState *new_bs) | ||
164 | + BlockDriverState *new_bs, | ||
165 | + bool free_empty_child) | ||
166 | { | ||
167 | BdrvChild *child = *childp; | ||
168 | BlockDriverState *old_bs = child->bs; | ||
169 | @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild **childp, | ||
170 | } | ||
171 | |||
172 | child->bs = new_bs; | ||
173 | + if (!new_bs) { | ||
174 | + *childp = NULL; | ||
175 | + } | ||
176 | |||
177 | if (new_bs) { | ||
178 | QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); | ||
179 | @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild **childp, | ||
180 | bdrv_parent_drained_end_single(child); | ||
181 | drain_saldo++; | ||
182 | } | ||
66 | + | 183 | + |
67 | def runMirror(self, sync): | 184 | + if (free_empty_child && !child->bs) { |
68 | if self.cmd == 'blockdev-mirror': | 185 | + bdrv_child_free(child); |
69 | result = self.vm.qmp(self.cmd, job_id='mirror-job', device='source', | 186 | + } |
70 | - sync=sync, target='target') | 187 | } |
71 | + sync=sync, target='target', | 188 | |
72 | + auto_finalize=False) | 189 | /** |
73 | else: | 190 | @@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque) |
74 | if self.existing: | 191 | BdrvChild *child = *s->child; |
75 | mode = 'existing' | 192 | BlockDriverState *bs = child->bs; |
76 | @@ -XXX,XX +XXX,XX @@ class MirrorBaseClass(BaseClass): | 193 | |
77 | result = self.vm.qmp(self.cmd, job_id='mirror-job', device='source', | 194 | - bdrv_replace_child_noperm(s->child, NULL); |
78 | sync=sync, target=target_img, | 195 | + /* |
79 | format=iotests.imgfmt, mode=mode, | 196 | + * Pass free_empty_child=false, because we still need the child |
80 | - node_name='target') | 197 | + * for the AioContext operations on the parent below; those |
81 | + node_name='target', auto_finalize=False) | 198 | + * BdrvChildClass methods all work on a BdrvChild object, so we |
82 | 199 | + * need to keep it as an empty shell (after this function, it will | |
83 | self.assert_qmp(result, 'return', {}) | 200 | + * not be attached to any parent, and it will not have a .bs). |
84 | 201 | + */ | |
85 | - self.complete_and_wait('mirror-job') | 202 | + bdrv_replace_child_noperm(s->child, NULL, false); |
86 | + self.vm.run_job('mirror-job', use_log=False, auto_finalize=False, | 203 | |
87 | + pre_finalize=self.openBacking, auto_dismiss=True) | 204 | if (bdrv_get_aio_context(bs) != s->old_child_ctx) { |
88 | 205 | bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort); | |
89 | def testFull(self): | 206 | @@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque) |
90 | self.runMirror('full') | 207 | |
91 | @@ -XXX,XX +XXX,XX @@ class TestBlockdevMirrorForcedBacking(MirrorBaseClass): | 208 | bdrv_unref(bs); |
92 | target_blockdev_backing = { 'driver': 'null-co' } | 209 | bdrv_child_free(child); |
93 | target_real_backing = 'null-co://' | 210 | - *s->child = NULL; |
94 | 211 | } | |
95 | +# Attach the backing chain only during completion, with blockdev-reopen | 212 | |
96 | +class TestBlockdevMirrorReopen(MirrorBaseClass): | 213 | static TransactionActionDrv bdrv_attach_child_common_drv = { |
97 | + cmd = 'blockdev-mirror' | 214 | @@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_common(BlockDriverState *child_bs, |
98 | + existing = True | 215 | } |
99 | + target_backing = 'null-co://' | 216 | |
100 | + target_open_with_backing = False | 217 | bdrv_ref(child_bs); |
101 | + | 218 | - bdrv_replace_child_noperm(&new_child, child_bs); |
102 | + def openBacking(self): | 219 | + bdrv_replace_child_noperm(&new_child, child_bs, true); |
103 | + if not self.target_open_with_backing: | 220 | + /* child_bs was non-NULL, so new_child must not have been freed */ |
104 | + result = self.vm.qmp('blockdev-add', node_name="backing", | 221 | + assert(new_child != NULL); |
105 | + driver="null-co") | 222 | |
106 | + self.assert_qmp(result, 'return', {}) | 223 | *child = new_child; |
107 | + result = self.vm.qmp('x-blockdev-reopen', node_name="target", | 224 | |
108 | + driver=iotests.imgfmt, file="target-file", | 225 | @@ -XXX,XX +XXX,XX @@ static void bdrv_detach_child(BdrvChild **childp) |
109 | + backing="backing") | 226 | { |
110 | + self.assert_qmp(result, 'return', {}) | 227 | BlockDriverState *old_bs = (*childp)->bs; |
111 | + | 228 | |
112 | +# Attach the backing chain only during completion, with blockdev-snapshot | 229 | - bdrv_replace_child_noperm(childp, NULL); |
113 | +class TestBlockdevMirrorSnapshot(MirrorBaseClass): | 230 | - bdrv_child_free(*childp); |
114 | + cmd = 'blockdev-mirror' | 231 | + bdrv_replace_child_noperm(childp, NULL, true); |
115 | + existing = True | 232 | |
116 | + target_backing = 'null-co://' | 233 | if (old_bs) { |
117 | + target_open_with_backing = False | 234 | /* |
118 | + | 235 | @@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, |
119 | + def openBacking(self): | 236 | } |
120 | + if not self.target_open_with_backing: | 237 | |
121 | + result = self.vm.qmp('blockdev-add', node_name="backing", | 238 | if (child->bs) { |
122 | + driver="null-co") | 239 | - bdrv_replace_child_tran(childp, NULL, tran); |
123 | + self.assert_qmp(result, 'return', {}) | 240 | + /* |
124 | + result = self.vm.qmp('blockdev-snapshot', node="backing", | 241 | + * Pass free_empty_child=false, we will free the child in |
125 | + overlay="target") | 242 | + * bdrv_remove_filter_or_cow_child_commit() |
126 | + self.assert_qmp(result, 'return', {}) | 243 | + */ |
127 | 244 | + bdrv_replace_child_tran(childp, NULL, tran, false); | |
128 | class TestCommit(BaseClass): | 245 | } |
129 | existing = False | 246 | |
130 | diff --git a/tests/qemu-iotests/155.out b/tests/qemu-iotests/155.out | 247 | s = g_new(BdrvRemoveFilterOrCowChild, 1); |
131 | index XXXXXXX..XXXXXXX 100644 | 248 | @@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, |
132 | --- a/tests/qemu-iotests/155.out | 249 | .is_backing = (childp == &bs->backing), |
133 | +++ b/tests/qemu-iotests/155.out | 250 | }; |
134 | @@ -XXX,XX +XXX,XX @@ | 251 | tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s); |
135 | -................... | 252 | - |
136 | +......................... | 253 | - *childp = NULL; |
137 | ---------------------------------------------------------------------- | 254 | } |
138 | -Ran 19 tests | 255 | |
139 | +Ran 25 tests | 256 | /* |
140 | 257 | @@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from, | |
141 | OK | 258 | * Passing a pointer to the local variable @c is fine here, because |
259 | * @to is not NULL, and so &c will not be attached to the transaction. | ||
260 | */ | ||
261 | - bdrv_replace_child_tran(&c, to, tran); | ||
262 | + bdrv_replace_child_tran(&c, to, tran, true); | ||
263 | } | ||
264 | |||
265 | return 0; | ||
266 | @@ -XXX,XX +XXX,XX @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs, | ||
267 | bdrv_drained_begin(old_bs); | ||
268 | bdrv_drained_begin(new_bs); | ||
269 | |||
270 | - bdrv_replace_child_tran(&child, new_bs, tran); | ||
271 | + bdrv_replace_child_tran(&child, new_bs, tran, true); | ||
272 | + /* @new_bs must have been non-NULL, so @child must not have been freed */ | ||
273 | + assert(child != NULL); | ||
274 | |||
275 | found = g_hash_table_new(NULL, NULL); | ||
276 | refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs); | ||
142 | -- | 277 | -- |
143 | 2.20.1 | 278 | 2.31.1 |
144 | 279 | ||
145 | 280 | diff view generated by jsdifflib |
1 | This patch adds test cases for attaching the backing chain to a mirror | 1 | From: Hanna Reitz <hreitz@redhat.com> |
---|---|---|---|
2 | job target right before finalising the job, where the image is in a | ||
3 | non-mainloop AioContext (i.e. the backing chain needs to be moved to the | ||
4 | AioContext of the mirror target). | ||
5 | 2 | ||
6 | This requires switching the test case from virtio-blk to virtio-scsi | 3 | See the comment for why this is necessary. |
7 | because virtio-blk only actually starts using the iothreads when the | ||
8 | guest driver initialises the device (which never happens in a test case | ||
9 | without a guest OS). virtio-scsi always keeps its block nodes in the | ||
10 | AioContext of the the requested iothread without guest interaction. | ||
11 | 4 | ||
12 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 5 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> |
13 | Message-Id: <20200310113831.27293-7-kwolf@redhat.com> | 6 | Message-Id: <20211111120829.81329-11-hreitz@redhat.com> |
14 | Reviewed-by: Peter Krempa <pkrempa@redhat.com> | ||
15 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 7 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
16 | --- | 8 | --- |
17 | tests/qemu-iotests/155 | 32 +++++++++++++++++++++++--------- | 9 | tests/qemu-iotests/030 | 11 ++++++++++- |
18 | tests/qemu-iotests/155.out | 4 ++-- | 10 | 1 file changed, 10 insertions(+), 1 deletion(-) |
19 | 2 files changed, 25 insertions(+), 11 deletions(-) | ||
20 | 11 | ||
21 | diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155 | 12 | diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 |
22 | index XXXXXXX..XXXXXXX 100755 | 13 | index XXXXXXX..XXXXXXX 100755 |
23 | --- a/tests/qemu-iotests/155 | 14 | --- a/tests/qemu-iotests/030 |
24 | +++ b/tests/qemu-iotests/155 | 15 | +++ b/tests/qemu-iotests/030 |
25 | @@ -XXX,XX +XXX,XX @@ target_img = os.path.join(iotests.test_dir, 'target.' + iotests.imgfmt) | 16 | @@ -XXX,XX +XXX,XX @@ class TestParallelOps(iotests.QMPTestCase): |
26 | # chain opened right away. If False, blockdev-add | 17 | speed=1024) |
27 | # opens it without a backing file and job completion | ||
28 | # is supposed to open the backing chain. | ||
29 | +# use_iothread: If True, an iothread is configured for the virtio-blk device | ||
30 | +# that uses the image being mirrored | ||
31 | |||
32 | class BaseClass(iotests.QMPTestCase): | ||
33 | target_blockdev_backing = None | ||
34 | target_real_backing = None | ||
35 | target_open_with_backing = True | ||
36 | + use_iothread = False | ||
37 | |||
38 | def setUp(self): | ||
39 | qemu_img('create', '-f', iotests.imgfmt, back0_img, '1440K') | ||
40 | @@ -XXX,XX +XXX,XX @@ class BaseClass(iotests.QMPTestCase): | ||
41 | 'file': {'driver': 'file', | ||
42 | 'filename': source_img}} | ||
43 | self.vm.add_blockdev(self.vm.qmp_to_opts(blockdev)) | ||
44 | - self.vm.add_device('virtio-blk,id=qdev0,drive=source') | ||
45 | + | ||
46 | + if self.use_iothread: | ||
47 | + self.vm.add_object('iothread,id=iothread0') | ||
48 | + iothread = ",iothread=iothread0" | ||
49 | + else: | ||
50 | + iothread = "" | ||
51 | + | ||
52 | + self.vm.add_device('virtio-scsi%s' % iothread) | ||
53 | + self.vm.add_device('scsi-hd,id=qdev0,drive=source') | ||
54 | + | ||
55 | self.vm.launch() | ||
56 | |||
57 | self.assertIntactSourceBackingChain() | ||
58 | @@ -XXX,XX +XXX,XX @@ class MirrorBaseClass(BaseClass): | ||
59 | def testFull(self): | ||
60 | self.runMirror('full') | ||
61 | |||
62 | - node = self.findBlockNode('target', | ||
63 | - '/machine/peripheral/qdev0/virtio-backend') | ||
64 | + node = self.findBlockNode('target', 'qdev0') | ||
65 | self.assertCorrectBackingImage(node, None) | ||
66 | self.assertIntactSourceBackingChain() | ||
67 | |||
68 | def testTop(self): | ||
69 | self.runMirror('top') | ||
70 | |||
71 | - node = self.findBlockNode('target', | ||
72 | - '/machine/peripheral/qdev0/virtio-backend') | ||
73 | + node = self.findBlockNode('target', 'qdev0') | ||
74 | self.assertCorrectBackingImage(node, back2_img) | ||
75 | self.assertIntactSourceBackingChain() | ||
76 | |||
77 | def testNone(self): | ||
78 | self.runMirror('none') | ||
79 | |||
80 | - node = self.findBlockNode('target', | ||
81 | - '/machine/peripheral/qdev0/virtio-backend') | ||
82 | + node = self.findBlockNode('target', 'qdev0') | ||
83 | self.assertCorrectBackingImage(node, source_img) | ||
84 | self.assertIntactSourceBackingChain() | ||
85 | |||
86 | @@ -XXX,XX +XXX,XX @@ class TestBlockdevMirrorReopen(MirrorBaseClass): | ||
87 | backing="backing") | ||
88 | self.assert_qmp(result, 'return', {}) | 18 | self.assert_qmp(result, 'return', {}) |
89 | 19 | ||
90 | +class TestBlockdevMirrorReopenIothread(TestBlockdevMirrorReopen): | 20 | - for job in pending_jobs: |
91 | + use_iothread = True | 21 | + # Do this in reverse: After unthrottling them, some jobs may finish |
92 | + | 22 | + # before we have unthrottled all of them. This will drain their |
93 | # Attach the backing chain only during completion, with blockdev-snapshot | 23 | + # subgraph, and this will make jobs above them advance (despite those |
94 | class TestBlockdevMirrorSnapshot(MirrorBaseClass): | 24 | + # jobs on top being throttled). In the worst case, all jobs below the |
95 | cmd = 'blockdev-mirror' | 25 | + # top one are finished before we can unthrottle it, and this makes it |
96 | @@ -XXX,XX +XXX,XX @@ class TestBlockdevMirrorSnapshot(MirrorBaseClass): | 26 | + # advance so far that it completes before we can unthrottle it - which |
97 | overlay="target") | 27 | + # results in an error. |
28 | + # Starting from the top (i.e. in reverse) does not have this problem: | ||
29 | + # When a job finishes, the ones below it are not advanced. | ||
30 | + for job in reversed(pending_jobs): | ||
31 | result = self.vm.qmp('block-job-set-speed', device=job, speed=0) | ||
98 | self.assert_qmp(result, 'return', {}) | 32 | self.assert_qmp(result, 'return', {}) |
99 | 33 | ||
100 | +class TestBlockdevMirrorSnapshotIothread(TestBlockdevMirrorSnapshot): | ||
101 | + use_iothread = True | ||
102 | + | ||
103 | class TestCommit(BaseClass): | ||
104 | existing = False | ||
105 | |||
106 | @@ -XXX,XX +XXX,XX @@ class TestCommit(BaseClass): | ||
107 | |||
108 | self.vm.event_wait('BLOCK_JOB_COMPLETED') | ||
109 | |||
110 | - node = self.findBlockNode(None, | ||
111 | - '/machine/peripheral/qdev0/virtio-backend') | ||
112 | + node = self.findBlockNode(None, 'qdev0') | ||
113 | self.assert_qmp(node, 'image' + '/backing-image' * 0 + '/filename', | ||
114 | back1_img) | ||
115 | self.assert_qmp(node, 'image' + '/backing-image' * 1 + '/filename', | ||
116 | diff --git a/tests/qemu-iotests/155.out b/tests/qemu-iotests/155.out | ||
117 | index XXXXXXX..XXXXXXX 100644 | ||
118 | --- a/tests/qemu-iotests/155.out | ||
119 | +++ b/tests/qemu-iotests/155.out | ||
120 | @@ -XXX,XX +XXX,XX @@ | ||
121 | -......................... | ||
122 | +............................... | ||
123 | ---------------------------------------------------------------------- | ||
124 | -Ran 25 tests | ||
125 | +Ran 31 tests | ||
126 | |||
127 | OK | ||
128 | -- | 34 | -- |
129 | 2.20.1 | 35 | 2.31.1 |
130 | 36 | ||
131 | 37 | diff view generated by jsdifflib |
1 | The 'job-complete' QMP command should be run with qmp() rather than | 1 | While introducing a non-QemuOpts code path for device creation for JSON |
---|---|---|---|
2 | qmp_log() if use_log=False is passed. | 2 | -device, we noticed that QMP device_add doesn't check its input |
3 | correctly (accepting arguments that should have been rejected), and that | ||
4 | users may be relying on this behaviour (libvirt did until it was fixed | ||
5 | recently). | ||
6 | |||
7 | Let's use a deprecation period before we fix this bug in QEMU to avoid | ||
8 | nasty surprises for users. | ||
3 | 9 | ||
4 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 10 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
5 | Message-Id: <20200310113831.27293-4-kwolf@redhat.com> | 11 | Message-Id: <20211111143530.18985-1-kwolf@redhat.com> |
6 | Reviewed-by: Peter Krempa <pkrempa@redhat.com> | 12 | Reviewed-by: Markus Armbruster <armbru@redhat.com> |
13 | Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> | ||
7 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 14 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
8 | --- | 15 | --- |
9 | tests/qemu-iotests/iotests.py | 5 ++++- | 16 | docs/about/deprecated.rst | 14 ++++++++++++++ |
10 | 1 file changed, 4 insertions(+), 1 deletion(-) | 17 | 1 file changed, 14 insertions(+) |
11 | 18 | ||
12 | diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py | 19 | diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst |
13 | index XXXXXXX..XXXXXXX 100644 | 20 | index XXXXXXX..XXXXXXX 100644 |
14 | --- a/tests/qemu-iotests/iotests.py | 21 | --- a/docs/about/deprecated.rst |
15 | +++ b/tests/qemu-iotests/iotests.py | 22 | +++ b/docs/about/deprecated.rst |
16 | @@ -XXX,XX +XXX,XX @@ class VM(qtest.QEMUQtestMachine): | 23 | @@ -XXX,XX +XXX,XX @@ options are removed in favor of using explicit ``blockdev-create`` and |
17 | if use_log: | 24 | ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for |
18 | log('Job failed: %s' % (j['error'])) | 25 | details. |
19 | elif status == 'ready': | 26 | |
20 | - self.qmp_log('job-complete', id=job) | 27 | +Incorrectly typed ``device_add`` arguments (since 6.2) |
21 | + if use_log: | 28 | +'''''''''''''''''''''''''''''''''''''''''''''''''''''' |
22 | + self.qmp_log('job-complete', id=job) | 29 | + |
23 | + else: | 30 | +Due to shortcomings in the internal implementation of ``device_add``, QEMU |
24 | + self.qmp('job-complete', id=job) | 31 | +incorrectly accepts certain invalid arguments: Any object or list arguments are |
25 | elif status == 'pending' and not auto_finalize: | 32 | +silently ignored. Other argument types are not checked, but an implicit |
26 | if pre_finalize: | 33 | +conversion happens, so that e.g. string values can be assigned to integer |
27 | pre_finalize() | 34 | +device properties or vice versa. |
35 | + | ||
36 | +This is a bug in QEMU that will be fixed in the future so that previously | ||
37 | +accepted incorrect commands will return an error. Users should make sure that | ||
38 | +all arguments passed to ``device_add`` are consistent with the documented | ||
39 | +property types. | ||
40 | + | ||
41 | System accelerators | ||
42 | ------------------- | ||
43 | |||
28 | -- | 44 | -- |
29 | 2.20.1 | 45 | 2.31.1 |
30 | 46 | ||
31 | 47 | diff view generated by jsdifflib |
1 | From: Daniel Henrique Barboza <danielhb413@gmail.com> | 1 | At the end of a reopen, we already call bdrv_refresh_limits(), which |
---|---|---|---|
2 | should update bs->request_alignment according to the new file | ||
3 | descriptor. However, raw_probe_alignment() relies on s->needs_alignment | ||
4 | and just uses 1 if it isn't set. We neglected to update this field, so | ||
5 | starting with cache=writeback and then reopening with cache=none means | ||
6 | that we get an incorrect bs->request_alignment == 1 and unaligned | ||
7 | requests fail instead of being automatically aligned. | ||
2 | 8 | ||
3 | Adding to Block Drivers the capability of being able to clean up | 9 | Fix this by recalculating s->needs_alignment in raw_refresh_limits() |
4 | its created files can be useful in certain situations. For the | 10 | before calling raw_probe_alignment(). |
5 | LUKS driver, for instance, a failure in one of its authentication | ||
6 | steps can leave files in the host that weren't there before. | ||
7 | 11 | ||
8 | This patch adds the 'bdrv_co_delete_file' interface to block | 12 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
9 | drivers and add it to the 'file' driver in file-posix.c. The | 13 | Message-Id: <20211104113109.56336-1-kwolf@redhat.com> |
10 | implementation is given by 'raw_co_delete_file'. | 14 | Reviewed-by: Hanna Reitz <hreitz@redhat.com> |
11 | 15 | Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> | |
12 | Suggested-by: Daniel P. Berrangé <berrange@redhat.com> | ||
13 | Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> | ||
14 | Message-Id: <20200130213907.2830642-2-danielhb413@gmail.com> | ||
15 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 16 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
16 | --- | 17 | --- |
17 | include/block/block_int.h | 4 ++++ | 18 | block/file-posix.c | 20 ++++++++++++++++---- |
18 | block/file-posix.c | 23 +++++++++++++++++++++++ | 19 | tests/qemu-iotests/142 | 22 ++++++++++++++++++++++ |
19 | 2 files changed, 27 insertions(+) | 20 | tests/qemu-iotests/142.out | 15 +++++++++++++++ |
21 | 3 files changed, 53 insertions(+), 4 deletions(-) | ||
20 | 22 | ||
21 | diff --git a/include/block/block_int.h b/include/block/block_int.h | ||
22 | index XXXXXXX..XXXXXXX 100644 | ||
23 | --- a/include/block/block_int.h | ||
24 | +++ b/include/block/block_int.h | ||
25 | @@ -XXX,XX +XXX,XX @@ struct BlockDriver { | ||
26 | */ | ||
27 | int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs); | ||
28 | |||
29 | + /* Delete a created file. */ | ||
30 | + int coroutine_fn (*bdrv_co_delete_file)(BlockDriverState *bs, | ||
31 | + Error **errp); | ||
32 | + | ||
33 | /* | ||
34 | * Flushes all data that was already written to the OS all the way down to | ||
35 | * the disk (for example file-posix.c calls fsync()). | ||
36 | diff --git a/block/file-posix.c b/block/file-posix.c | 23 | diff --git a/block/file-posix.c b/block/file-posix.c |
37 | index XXXXXXX..XXXXXXX 100644 | 24 | index XXXXXXX..XXXXXXX 100644 |
38 | --- a/block/file-posix.c | 25 | --- a/block/file-posix.c |
39 | +++ b/block/file-posix.c | 26 | +++ b/block/file-posix.c |
40 | @@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, | 27 | @@ -XXX,XX +XXX,XX @@ typedef struct BDRVRawState { |
41 | return raw_co_create(&options, errp); | 28 | int page_cache_inconsistent; /* errno from fdatasync failure */ |
29 | bool has_fallocate; | ||
30 | bool needs_alignment; | ||
31 | + bool force_alignment; | ||
32 | bool drop_cache; | ||
33 | bool check_cache_dropped; | ||
34 | struct { | ||
35 | @@ -XXX,XX +XXX,XX @@ static bool dio_byte_aligned(int fd) | ||
36 | return false; | ||
42 | } | 37 | } |
43 | 38 | ||
44 | +static int coroutine_fn raw_co_delete_file(BlockDriverState *bs, | 39 | +static bool raw_needs_alignment(BlockDriverState *bs) |
45 | + Error **errp) | ||
46 | +{ | 40 | +{ |
47 | + struct stat st; | 41 | + BDRVRawState *s = bs->opaque; |
48 | + int ret; | ||
49 | + | 42 | + |
50 | + if (!(stat(bs->filename, &st) == 0) || !S_ISREG(st.st_mode)) { | 43 | + if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) { |
51 | + error_setg_errno(errp, ENOENT, "%s is not a regular file", | 44 | + return true; |
52 | + bs->filename); | ||
53 | + return -ENOENT; | ||
54 | + } | 45 | + } |
55 | + | 46 | + |
56 | + ret = unlink(bs->filename); | 47 | + return s->force_alignment; |
57 | + if (ret < 0) { | ||
58 | + ret = -errno; | ||
59 | + error_setg_errno(errp, -ret, "Error when deleting file %s", | ||
60 | + bs->filename); | ||
61 | + } | ||
62 | + | ||
63 | + return ret; | ||
64 | +} | 48 | +} |
65 | + | 49 | + |
66 | /* | 50 | /* Check if read is allowed with given memory buffer and length. |
67 | * Find allocation range in @bs around offset @start. | 51 | * |
68 | * May change underlying file descriptor's file offset. | 52 | * This function is used to check O_DIRECT memory buffer and request alignment. |
69 | @@ -XXX,XX +XXX,XX @@ BlockDriver bdrv_file = { | 53 | @@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options, |
70 | .bdrv_co_block_status = raw_co_block_status, | 54 | |
71 | .bdrv_co_invalidate_cache = raw_co_invalidate_cache, | 55 | s->has_discard = true; |
72 | .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes, | 56 | s->has_write_zeroes = true; |
73 | + .bdrv_co_delete_file = raw_co_delete_file, | 57 | - if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) { |
74 | 58 | - s->needs_alignment = true; | |
75 | .bdrv_co_preadv = raw_co_preadv, | 59 | - } |
76 | .bdrv_co_pwritev = raw_co_pwritev, | 60 | |
61 | if (fstat(s->fd, &st) < 0) { | ||
62 | ret = -errno; | ||
63 | @@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options, | ||
64 | * so QEMU makes sure all IO operations on the device are aligned | ||
65 | * to sector size, or else FreeBSD will reject them with EINVAL. | ||
66 | */ | ||
67 | - s->needs_alignment = true; | ||
68 | + s->force_alignment = true; | ||
69 | } | ||
70 | #endif | ||
71 | + s->needs_alignment = raw_needs_alignment(bs); | ||
72 | |||
73 | #ifdef CONFIG_XFS | ||
74 | if (platform_test_xfs_fd(s->fd)) { | ||
75 | @@ -XXX,XX +XXX,XX @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) | ||
76 | BDRVRawState *s = bs->opaque; | ||
77 | struct stat st; | ||
78 | |||
79 | + s->needs_alignment = raw_needs_alignment(bs); | ||
80 | raw_probe_alignment(bs, s->fd, errp); | ||
81 | + | ||
82 | bs->bl.min_mem_alignment = s->buf_align; | ||
83 | bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size); | ||
84 | |||
85 | diff --git a/tests/qemu-iotests/142 b/tests/qemu-iotests/142 | ||
86 | index XXXXXXX..XXXXXXX 100755 | ||
87 | --- a/tests/qemu-iotests/142 | ||
88 | +++ b/tests/qemu-iotests/142 | ||
89 | @@ -XXX,XX +XXX,XX @@ info block backing-file" | ||
90 | |||
91 | echo "$hmp_cmds" | run_qemu -drive "$files","$ids" | grep "Cache" | ||
92 | |||
93 | +echo | ||
94 | +echo "--- Alignment after changing O_DIRECT ---" | ||
95 | +echo | ||
96 | + | ||
97 | +# Directly test the protocol level: Can unaligned requests succeed even if | ||
98 | +# O_DIRECT was only enabled through a reopen and vice versa? | ||
99 | + | ||
100 | +$QEMU_IO --cache=writeback -f file $TEST_IMG <<EOF | _filter_qemu_io | ||
101 | +read 42 42 | ||
102 | +reopen -o cache.direct=on | ||
103 | +read 42 42 | ||
104 | +reopen -o cache.direct=off | ||
105 | +read 42 42 | ||
106 | +EOF | ||
107 | +$QEMU_IO --cache=none -f file $TEST_IMG <<EOF | _filter_qemu_io | ||
108 | +read 42 42 | ||
109 | +reopen -o cache.direct=off | ||
110 | +read 42 42 | ||
111 | +reopen -o cache.direct=on | ||
112 | +read 42 42 | ||
113 | +EOF | ||
114 | + | ||
115 | # success, all done | ||
116 | echo "*** done" | ||
117 | rm -f $seq.full | ||
118 | diff --git a/tests/qemu-iotests/142.out b/tests/qemu-iotests/142.out | ||
119 | index XXXXXXX..XXXXXXX 100644 | ||
120 | --- a/tests/qemu-iotests/142.out | ||
121 | +++ b/tests/qemu-iotests/142.out | ||
122 | @@ -XXX,XX +XXX,XX @@ cache.no-flush=on on backing-file | ||
123 | Cache mode: writeback | ||
124 | Cache mode: writeback, direct | ||
125 | Cache mode: writeback, ignore flushes | ||
126 | + | ||
127 | +--- Alignment after changing O_DIRECT --- | ||
128 | + | ||
129 | +read 42/42 bytes at offset 42 | ||
130 | +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) | ||
131 | +read 42/42 bytes at offset 42 | ||
132 | +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) | ||
133 | +read 42/42 bytes at offset 42 | ||
134 | +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) | ||
135 | +read 42/42 bytes at offset 42 | ||
136 | +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) | ||
137 | +read 42/42 bytes at offset 42 | ||
138 | +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) | ||
139 | +read 42/42 bytes at offset 42 | ||
140 | +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) | ||
141 | *** done | ||
77 | -- | 142 | -- |
78 | 2.20.1 | 143 | 2.31.1 |
79 | 144 | ||
80 | 145 | diff view generated by jsdifflib |
1 | From: Pan Nengyuan <pannengyuan@huawei.com> | 1 | From: Stefan Hajnoczi <stefanha@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | 'type/id' forgot to free in qmp_object_add, this patch fix that. | 3 | Reported by Coverity (CID 1465222). |
4 | 4 | ||
5 | The leak stack: | 5 | Fixes: 4a1d937796de0fecd8b22d7dbebf87f38e8282fd ("softmmu/qdev-monitor: add error handling in qdev_set_id") |
6 | Direct leak of 84 byte(s) in 6 object(s) allocated from: | 6 | Cc: Damien Hedde <damien.hedde@greensocs.com> |
7 | #0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768) | 7 | Cc: Kevin Wolf <kwolf@redhat.com> |
8 | #1 0x7fe2a5044445 in g_malloc (/lib64/libglib-2.0.so.0+0x52445) | 8 | Cc: Michael S. Tsirkin <mst@redhat.com> |
9 | #2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92) | 9 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
10 | #3 0x56344954e692 in qmp_object_add /mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:258 | 10 | Message-Id: <20211102163342.31162-1-stefanha@redhat.com> |
11 | #4 0x563449960f5a in do_qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132 | 11 | Reviewed-by: Kevin Wolf <kwolf@redhat.com> |
12 | #5 0x563449960f5a in qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175 | 12 | Reviewed-by: Michael S. Tsirkin <mst@redhat.com> |
13 | #6 0x563449498a30 in monitor_qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145 | 13 | Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> |
14 | #7 0x56344949a64f in monitor_qmp_bh_dispatcher /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234 | 14 | Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> |
15 | #8 0x563449a92a3a in aio_bh_call /mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136 | 15 | Reviewed-by: Damien Hedde <damien.hedde@greensocs.com> |
16 | 16 | Reviewed-by: Markus Armbruster <armbru@redhat.com> | |
17 | Direct leak of 54 byte(s) in 6 object(s) allocated from: | ||
18 | #0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768) | ||
19 | #1 0x7fe2a5044445 in g_malloc (/lib64/libglib-2.0.so.0+0x52445) | ||
20 | #2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92) | ||
21 | #3 0x56344954e6c4 in qmp_object_add /mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:267 | ||
22 | #4 0x563449960f5a in do_qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132 | ||
23 | #5 0x563449960f5a in qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175 | ||
24 | #6 0x563449498a30 in monitor_qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145 | ||
25 | #7 0x56344949a64f in monitor_qmp_bh_dispatcher /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234 | ||
26 | #8 0x563449a92a3a in aio_bh_call /mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136 | ||
27 | |||
28 | Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e | ||
29 | Reported-by: Euler Robot <euler.robot@huawei.com> | ||
30 | Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> | ||
31 | Message-Id: <20200310064640.5059-1-pannengyuan@huawei.com> | ||
32 | Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> | ||
33 | Acked-by: Igor Mammedov <imammedo@redhat.com> | ||
34 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | 17 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
35 | --- | 18 | --- |
36 | qom/qom-qmp-cmds.c | 16 ++++++---------- | 19 | softmmu/qdev-monitor.c | 2 +- |
37 | 1 file changed, 6 insertions(+), 10 deletions(-) | 20 | 1 file changed, 1 insertion(+), 1 deletion(-) |
38 | 21 | ||
39 | diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c | 22 | diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c |
40 | index XXXXXXX..XXXXXXX 100644 | 23 | index XXXXXXX..XXXXXXX 100644 |
41 | --- a/qom/qom-qmp-cmds.c | 24 | --- a/softmmu/qdev-monitor.c |
42 | +++ b/qom/qom-qmp-cmds.c | 25 | +++ b/softmmu/qdev-monitor.c |
43 | @@ -XXX,XX +XXX,XX @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp) | 26 | @@ -XXX,XX +XXX,XX @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) |
44 | QDict *pdict; | 27 | if (prop) { |
45 | Visitor *v; | 28 | dev->id = id; |
46 | Object *obj; | 29 | } else { |
47 | - const char *type; | 30 | - g_free(id); |
48 | - const char *id; | 31 | error_setg(errp, "Duplicate device ID '%s'", id); |
49 | + g_autofree char *type = NULL; | 32 | + g_free(id); |
50 | + g_autofree char *id = NULL; | 33 | return NULL; |
51 | 34 | } | |
52 | - type = qdict_get_try_str(qdict, "qom-type"); | 35 | } else { |
53 | + type = g_strdup(qdict_get_try_str(qdict, "qom-type")); | ||
54 | if (!type) { | ||
55 | error_setg(errp, QERR_MISSING_PARAMETER, "qom-type"); | ||
56 | return; | ||
57 | - } else { | ||
58 | - type = g_strdup(type); | ||
59 | - qdict_del(qdict, "qom-type"); | ||
60 | } | ||
61 | + qdict_del(qdict, "qom-type"); | ||
62 | |||
63 | - id = qdict_get_try_str(qdict, "id"); | ||
64 | + id = g_strdup(qdict_get_try_str(qdict, "id")); | ||
65 | if (!id) { | ||
66 | error_setg(errp, QERR_MISSING_PARAMETER, "id"); | ||
67 | return; | ||
68 | - } else { | ||
69 | - id = g_strdup(id); | ||
70 | - qdict_del(qdict, "id"); | ||
71 | } | ||
72 | + qdict_del(qdict, "id"); | ||
73 | |||
74 | props = qdict_get(qdict, "props"); | ||
75 | if (props) { | ||
76 | -- | 36 | -- |
77 | 2.20.1 | 37 | 2.31.1 |
78 | 38 | ||
79 | 39 | diff view generated by jsdifflib |