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