1 | The following changes since commit 64175afc695c0672876fbbfc31b299c86d562cb4: | 1 | The following changes since commit 42f6c9179be4401974dd3a75ee72defd16b5092d: |
---|---|---|---|
2 | 2 | ||
3 | arm_gicv3: Fix ICC_BPR1 reset value when EL3 not implemented (2017-06-07 17:21:44 +0100) | 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://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request | 7 | https://gitlab.com/hreitz/qemu.git tags/pull-block-2021-11-16 |
8 | 8 | ||
9 | for you to fetch changes up to 56faeb9bb6872b3f926b3b3e0452a70beea10af2: | 9 | for you to fetch changes up to 5dbd0ce115c7720268e653fe928bb6a0c1314a80: |
10 | 10 | ||
11 | block/gluster.c: Handle qdict_array_entries() failure (2017-06-09 08:41:29 -0400) | 11 | file-posix: Fix alignment after reopen changing O_DIRECT (2021-11-16 11:30:29 +0100) |
12 | 12 | ||
13 | ---------------------------------------------------------------- | 13 | ---------------------------------------------------------------- |
14 | Gluster patch | 14 | Block patches for 6.2.0-rc1: |
15 | - Fixes to image streaming job and block layer reconfiguration to make | ||
16 | iotest 030 pass again | ||
17 | - docs: Deprecate incorrectly typed device_add arguments | ||
18 | - file-posix: Fix alignment after reopen changing O_DIRECT | ||
19 | |||
15 | ---------------------------------------------------------------- | 20 | ---------------------------------------------------------------- |
21 | v2: | ||
22 | - Fixed iotest 142 (modified by "file-posix: Fix alignment after reopen | ||
23 | changing O_DIRECT") -- at least I hope so: for me, it now passes on a | ||
24 | 4k block device, and the gitlab pipeline passed, too | ||
16 | 25 | ||
17 | Peter Maydell (1): | 26 | - Note that because I had to modify Kevin's pull request, I did not want |
18 | block/gluster.c: Handle qdict_array_entries() failure | 27 | to merge it partially (with a merge commit), but instead decided to |
28 | apply all patches from the pull request mails (including their message | ||
29 | IDs) | ||
19 | 30 | ||
20 | block/gluster.c | 3 +-- | 31 | ---------------------------------------------------------------- |
21 | 1 file changed, 1 insertion(+), 2 deletions(-) | 32 | Hanna Reitz (10): |
33 | stream: Traverse graph after modification | ||
34 | block: Manipulate children list in .attach/.detach | ||
35 | block: Unite remove_empty_child and child_free | ||
36 | block: Drop detached child from ignore list | ||
37 | block: Pass BdrvChild ** to replace_child_noperm | ||
38 | block: Restructure remove_file_or_backing_child() | ||
39 | transactions: Invoke clean() after everything else | ||
40 | block: Let replace_child_tran keep indirect pointer | ||
41 | block: Let replace_child_noperm free children | ||
42 | iotests/030: Unthrottle parallel jobs in reverse | ||
43 | |||
44 | Kevin Wolf (2): | ||
45 | docs: Deprecate incorrectly typed device_add arguments | ||
46 | file-posix: Fix alignment after reopen changing O_DIRECT | ||
47 | |||
48 | Stefan Hajnoczi (1): | ||
49 | softmmu/qdev-monitor: fix use-after-free in qdev_set_id() | ||
50 | |||
51 | docs/about/deprecated.rst | 14 +++ | ||
52 | include/qemu/transactions.h | 3 + | ||
53 | block.c | 233 +++++++++++++++++++++++++++--------- | ||
54 | block/file-posix.c | 20 +++- | ||
55 | block/stream.c | 7 +- | ||
56 | softmmu/qdev-monitor.c | 2 +- | ||
57 | util/transactions.c | 8 +- | ||
58 | tests/qemu-iotests/030 | 11 +- | ||
59 | tests/qemu-iotests/142 | 29 +++++ | ||
60 | tests/qemu-iotests/142.out | 18 +++ | ||
61 | 10 files changed, 279 insertions(+), 66 deletions(-) | ||
22 | 62 | ||
23 | -- | 63 | -- |
24 | 2.9.3 | 64 | 2.33.1 |
25 | 65 | ||
26 | 66 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | bdrv_cor_filter_drop() modifies the block graph. That means that other | ||
2 | parties can also modify the block graph before it returns. Therefore, | ||
3 | we cannot assume that the result of a graph traversal we did before | ||
4 | remains valid afterwards. | ||
1 | 5 | ||
6 | We should thus fetch `base` and `unfiltered_base` afterwards instead of | ||
7 | before. | ||
8 | |||
9 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
10 | Reviewed-by: Kevin Wolf <kwolf@redhat.com> | ||
11 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
12 | Message-Id: <20211111120829.81329-2-hreitz@redhat.com> | ||
13 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
14 | Message-Id: <20211115145409.176785-2-kwolf@redhat.com> | ||
15 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
16 | --- | ||
17 | block/stream.c | 7 +++++-- | ||
18 | 1 file changed, 5 insertions(+), 2 deletions(-) | ||
19 | |||
20 | diff --git a/block/stream.c b/block/stream.c | ||
21 | index XXXXXXX..XXXXXXX 100644 | ||
22 | --- a/block/stream.c | ||
23 | +++ b/block/stream.c | ||
24 | @@ -XXX,XX +XXX,XX @@ static int stream_prepare(Job *job) | ||
25 | { | ||
26 | StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); | ||
27 | BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs); | ||
28 | - BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base); | ||
29 | - BlockDriverState *unfiltered_base = bdrv_skip_filters(base); | ||
30 | + BlockDriverState *base; | ||
31 | + BlockDriverState *unfiltered_base; | ||
32 | Error *local_err = NULL; | ||
33 | int ret = 0; | ||
34 | |||
35 | @@ -XXX,XX +XXX,XX @@ static int stream_prepare(Job *job) | ||
36 | bdrv_cor_filter_drop(s->cor_filter_bs); | ||
37 | s->cor_filter_bs = NULL; | ||
38 | |||
39 | + base = bdrv_filter_or_cow_bs(s->above_base); | ||
40 | + unfiltered_base = bdrv_skip_filters(base); | ||
41 | + | ||
42 | if (bdrv_cow_child(unfiltered_bs)) { | ||
43 | const char *base_id = NULL, *base_fmt = NULL; | ||
44 | if (unfiltered_base) { | ||
45 | -- | ||
46 | 2.33.1 | ||
47 | |||
48 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | The children list is specific to BDS parents. We should not modify it | ||
2 | in the general children modification code, but let BDS parents deal with | ||
3 | it in their .attach() and .detach() methods. | ||
1 | 4 | ||
5 | This also has the advantage that a BdrvChild is removed from the | ||
6 | children list before its .bs pointer can become NULL. BDS parents | ||
7 | generally assume that their children's .bs pointer is never NULL, so | ||
8 | this is actually a bug fix. | ||
9 | |||
10 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
11 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
12 | Message-Id: <20211111120829.81329-3-hreitz@redhat.com> | ||
13 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
14 | Message-Id: <20211115145409.176785-3-kwolf@redhat.com> | ||
15 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
16 | --- | ||
17 | block.c | 14 +++++--------- | ||
18 | 1 file changed, 5 insertions(+), 9 deletions(-) | ||
19 | |||
20 | diff --git a/block.c b/block.c | ||
21 | index XXXXXXX..XXXXXXX 100644 | ||
22 | --- a/block.c | ||
23 | +++ b/block.c | ||
24 | @@ -XXX,XX +XXX,XX @@ static void bdrv_child_cb_attach(BdrvChild *child) | ||
25 | { | ||
26 | BlockDriverState *bs = child->opaque; | ||
27 | |||
28 | + QLIST_INSERT_HEAD(&bs->children, child, next); | ||
29 | + | ||
30 | if (child->role & BDRV_CHILD_COW) { | ||
31 | bdrv_backing_attach(child); | ||
32 | } | ||
33 | @@ -XXX,XX +XXX,XX @@ static void bdrv_child_cb_detach(BdrvChild *child) | ||
34 | } | ||
35 | |||
36 | bdrv_unapply_subtree_drain(child, bs); | ||
37 | + | ||
38 | + QLIST_REMOVE(child, next); | ||
39 | } | ||
40 | |||
41 | static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base, | ||
42 | @@ -XXX,XX +XXX,XX @@ static void bdrv_child_free(void *opaque) | ||
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 { | ||
80 | -- | ||
81 | 2.33.1 | ||
82 | |||
83 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | Now that bdrv_remove_empty_child() no longer removes the child from the | ||
2 | parent's children list but only checks that it is not in such a list, it | ||
3 | is only a wrapper around bdrv_child_free() that checks that the child is | ||
4 | empty and unused. That should apply to all children that we free, so | ||
5 | put those checks into bdrv_child_free() and drop | ||
6 | bdrv_remove_empty_child(). | ||
1 | 7 | ||
8 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
9 | Reviewed-by: Kevin Wolf <kwolf@redhat.com> | ||
10 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
11 | Message-Id: <20211111120829.81329-4-hreitz@redhat.com> | ||
12 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
13 | Message-Id: <20211115145409.176785-4-kwolf@redhat.com> | ||
14 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
15 | --- | ||
16 | block.c | 26 +++++++++++++------------- | ||
17 | 1 file changed, 13 insertions(+), 13 deletions(-) | ||
18 | |||
19 | diff --git a/block.c b/block.c | ||
20 | index XXXXXXX..XXXXXXX 100644 | ||
21 | --- a/block.c | ||
22 | +++ b/block.c | ||
23 | @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild *child, | ||
24 | } | ||
25 | } | ||
26 | |||
27 | -static void bdrv_child_free(void *opaque) | ||
28 | -{ | ||
29 | - BdrvChild *c = opaque; | ||
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) | ||
43 | { | ||
44 | assert(!child->bs); | ||
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 | /* | ||
80 | -- | ||
81 | 2.33.1 | ||
82 | |||
83 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | bdrv_attach_child_common_abort() restores the parent's AioContext. To | ||
2 | do so, the child (which was supposed to be attached, but is now detached | ||
3 | again by this abort handler) is added to the ignore list for the | ||
4 | AioContext changing functions. | ||
1 | 5 | ||
6 | However, since we modify a BDS's children list in the BdrvChildClass's | ||
7 | .attach and .detach handlers, the child is already effectively detached | ||
8 | from the parent by this point. We do not need to put it into the ignore | ||
9 | list. | ||
10 | |||
11 | Use this opportunity to clean up the empty line structure: Keep setting | ||
12 | the ignore list, invoking the AioContext function, and freeing the | ||
13 | ignore list in blocks separated by empty lines. | ||
14 | |||
15 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
16 | Reviewed-by: Kevin Wolf <kwolf@redhat.com> | ||
17 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
18 | Message-Id: <20211111120829.81329-5-hreitz@redhat.com> | ||
19 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
20 | Message-Id: <20211115145409.176785-5-kwolf@redhat.com> | ||
21 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
22 | --- | ||
23 | block.c | 8 +++++--- | ||
24 | 1 file changed, 5 insertions(+), 3 deletions(-) | ||
25 | |||
26 | diff --git a/block.c b/block.c | ||
27 | index XXXXXXX..XXXXXXX 100644 | ||
28 | --- a/block.c | ||
29 | +++ b/block.c | ||
30 | @@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque) | ||
31 | } | ||
32 | |||
33 | if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) { | ||
34 | - GSList *ignore = g_slist_prepend(NULL, child); | ||
35 | + GSList *ignore; | ||
36 | |||
37 | + /* No need to ignore `child`, because it has been detached already */ | ||
38 | + ignore = NULL; | ||
39 | child->klass->can_set_aio_ctx(child, s->old_parent_ctx, &ignore, | ||
40 | &error_abort); | ||
41 | g_slist_free(ignore); | ||
42 | - ignore = g_slist_prepend(NULL, child); | ||
43 | - child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore); | ||
44 | |||
45 | + ignore = NULL; | ||
46 | + child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore); | ||
47 | g_slist_free(ignore); | ||
48 | } | ||
49 | |||
50 | -- | ||
51 | 2.33.1 | ||
52 | |||
53 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | bdrv_replace_child_noperm() modifies BdrvChild.bs, and can potentially | ||
2 | set it to NULL. That is dangerous, because BDS parents generally assume | ||
3 | that their children's .bs pointer is never NULL. We therefore want to | ||
4 | let bdrv_replace_child_noperm() set the corresponding BdrvChild pointer | ||
5 | to NULL, too. | ||
1 | 6 | ||
7 | This patch lays the foundation for it by passing a BdrvChild ** pointer | ||
8 | to bdrv_replace_child_noperm() so that it can later use it to NULL the | ||
9 | BdrvChild pointer immediately after setting BdrvChild.bs to NULL. | ||
10 | |||
11 | (We will still need to undertake some intermediate steps, though.) | ||
12 | |||
13 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
14 | Message-Id: <20211111120829.81329-6-hreitz@redhat.com> | ||
15 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
16 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
17 | Message-Id: <20211115145409.176785-6-kwolf@redhat.com> | ||
18 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
19 | --- | ||
20 | block.c | 23 ++++++++++++----------- | ||
21 | 1 file changed, 12 insertions(+), 11 deletions(-) | ||
22 | |||
23 | diff --git a/block.c b/block.c | ||
24 | index XXXXXXX..XXXXXXX 100644 | ||
25 | --- a/block.c | ||
26 | +++ b/block.c | ||
27 | @@ -XXX,XX +XXX,XX @@ static BlockDriverState *bdrv_open_inherit(const char *filename, | ||
28 | static bool bdrv_recurse_has_child(BlockDriverState *bs, | ||
29 | BlockDriverState *child); | ||
30 | |||
31 | -static void bdrv_replace_child_noperm(BdrvChild *child, | ||
32 | +static void bdrv_replace_child_noperm(BdrvChild **child, | ||
33 | BlockDriverState *new_bs); | ||
34 | static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, | ||
35 | BdrvChild *child, | ||
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); | ||
48 | } | ||
49 | - bdrv_replace_child_noperm(child, new_bs); | ||
50 | + bdrv_replace_child_noperm(&child, new_bs); | ||
51 | /* old_bs reference is transparently moved from @child to @s */ | ||
52 | } | ||
53 | |||
54 | @@ -XXX,XX +XXX,XX @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm) | ||
55 | return permissions[qapi_perm]; | ||
56 | } | ||
57 | |||
58 | -static void bdrv_replace_child_noperm(BdrvChild *child, | ||
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, | ||
76 | } | ||
77 | |||
78 | bdrv_ref(child_bs); | ||
79 | - bdrv_replace_child_noperm(new_child, child_bs); | ||
80 | + bdrv_replace_child_noperm(&new_child, child_bs); | ||
81 | |||
82 | *child = new_child; | ||
83 | |||
84 | @@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, | ||
85 | return 0; | ||
86 | } | ||
87 | |||
88 | -static void bdrv_detach_child(BdrvChild *child) | ||
89 | +static void bdrv_detach_child(BdrvChild **childp) | ||
90 | { | ||
91 | - BlockDriverState *old_bs = child->bs; | ||
92 | + BlockDriverState *old_bs = (*childp)->bs; | ||
93 | |||
94 | - bdrv_replace_child_noperm(child, NULL); | ||
95 | - bdrv_child_free(child); | ||
96 | + bdrv_replace_child_noperm(childp, NULL); | ||
97 | + bdrv_child_free(*childp); | ||
98 | |||
99 | if (old_bs) { | ||
100 | /* | ||
101 | @@ -XXX,XX +XXX,XX @@ void bdrv_root_unref_child(BdrvChild *child) | ||
102 | BlockDriverState *child_bs; | ||
103 | |||
104 | child_bs = child->bs; | ||
105 | - bdrv_detach_child(child); | ||
106 | + bdrv_detach_child(&child); | ||
107 | bdrv_unref(child_bs); | ||
108 | } | ||
109 | |||
110 | -- | ||
111 | 2.33.1 | ||
112 | |||
113 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | As of a future patch, bdrv_replace_child_tran() will take a BdrvChild ** | ||
2 | pointer. Prepare for that by getting such a pointer and using it where | ||
3 | applicable, and (dereferenced) as a parameter for | ||
4 | bdrv_replace_child_tran(). | ||
1 | 5 | ||
6 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
7 | Message-Id: <20211111120829.81329-7-hreitz@redhat.com> | ||
8 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
9 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
10 | Message-Id: <20211115145409.176785-7-kwolf@redhat.com> | ||
11 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
12 | --- | ||
13 | block.c | 21 ++++++++++++--------- | ||
14 | 1 file changed, 12 insertions(+), 9 deletions(-) | ||
15 | |||
16 | diff --git a/block.c b/block.c | ||
17 | index XXXXXXX..XXXXXXX 100644 | ||
18 | --- a/block.c | ||
19 | +++ b/block.c | ||
20 | @@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, | ||
21 | BdrvChild *child, | ||
22 | Transaction *tran) | ||
23 | { | ||
24 | + BdrvChild **childp; | ||
25 | BdrvRemoveFilterOrCowChild *s; | ||
26 | |||
27 | - assert(child == bs->backing || child == bs->file); | ||
28 | - | ||
29 | if (!child) { | ||
30 | return; | ||
31 | } | ||
32 | |||
33 | + if (child == bs->backing) { | ||
34 | + childp = &bs->backing; | ||
35 | + } else if (child == bs->file) { | ||
36 | + childp = &bs->file; | ||
37 | + } else { | ||
38 | + g_assert_not_reached(); | ||
39 | + } | ||
40 | + | ||
41 | if (child->bs) { | ||
42 | - bdrv_replace_child_tran(child, NULL, tran); | ||
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 | /* | ||
63 | -- | ||
64 | 2.33.1 | ||
65 | |||
66 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | Invoke the transaction drivers' .clean() methods only after all | ||
2 | .commit() or .abort() handlers are done. | ||
1 | 3 | ||
4 | This makes it easier to have nested transactions where the top-level | ||
5 | transactions pass objects to lower transactions that the latter can | ||
6 | still use throughout their commit/abort phases, while the top-level | ||
7 | transaction keeps a reference that is released in its .clean() method. | ||
8 | |||
9 | (Before this commit, that is also possible, but the top-level | ||
10 | transaction would need to take care to invoke tran_add() before the | ||
11 | lower-level transaction does. This commit makes the ordering | ||
12 | irrelevant, which is just a bit nicer.) | ||
13 | |||
14 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
15 | Message-Id: <20211111120829.81329-8-hreitz@redhat.com> | ||
16 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
17 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
18 | Message-Id: <20211115145409.176785-8-kwolf@redhat.com> | ||
19 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
20 | --- | ||
21 | include/qemu/transactions.h | 3 +++ | ||
22 | util/transactions.c | 8 ++++++-- | ||
23 | 2 files changed, 9 insertions(+), 2 deletions(-) | ||
24 | |||
25 | diff --git a/include/qemu/transactions.h b/include/qemu/transactions.h | ||
26 | index XXXXXXX..XXXXXXX 100644 | ||
27 | --- a/include/qemu/transactions.h | ||
28 | +++ b/include/qemu/transactions.h | ||
29 | @@ -XXX,XX +XXX,XX @@ | ||
30 | * tran_create(), call your "prepare" functions on it, and finally call | ||
31 | * tran_abort() or tran_commit() to finalize the transaction by corresponding | ||
32 | * finalization actions in reverse order. | ||
33 | + * | ||
34 | + * The clean() functions registered by the drivers in a transaction are called | ||
35 | + * last, after all abort() or commit() functions have been called. | ||
36 | */ | ||
37 | |||
38 | #ifndef QEMU_TRANSACTIONS_H | ||
39 | diff --git a/util/transactions.c b/util/transactions.c | ||
40 | index XXXXXXX..XXXXXXX 100644 | ||
41 | --- a/util/transactions.c | ||
42 | +++ b/util/transactions.c | ||
43 | @@ -XXX,XX +XXX,XX @@ void tran_abort(Transaction *tran) | ||
44 | { | ||
45 | TransactionAction *act, *next; | ||
46 | |||
47 | - QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) { | ||
48 | + QSLIST_FOREACH(act, &tran->actions, entry) { | ||
49 | if (act->drv->abort) { | ||
50 | act->drv->abort(act->opaque); | ||
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 | } | ||
73 | -- | ||
74 | 2.33.1 | ||
75 | |||
76 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | 1 | As of a future commit, bdrv_replace_child_noperm() will clear the | |
2 | indirect BdrvChild pointer passed to it if the new child BDS is NULL. | ||
3 | bdrv_replace_child_tran() will want to let it do that, but revert this | ||
4 | change in its abort handler. For that, we need to have it receive a | ||
5 | BdrvChild ** pointer, too, and keep it stored in the | ||
6 | BdrvReplaceChildState object that we attach to the transaction. | ||
7 | |||
8 | Note that we do not need to store it in the BdrvReplaceChildState when | ||
9 | new_bs is not NULL, because then there is nothing to revert. This is | ||
10 | important so that bdrv_replace_node_noperm() can pass a pointer to a | ||
11 | loop-local variable to bdrv_replace_child_tran() without worrying that | ||
12 | this pointer will outlive one loop iteration. | ||
13 | |||
14 | (Of course, for that to work, bdrv_replace_node_noperm() and in turn | ||
15 | bdrv_replace_node() and its relatives may not be called with a NULL @to | ||
16 | node. Luckily, they already are not, but now we should assert this.) | ||
17 | |||
18 | bdrv_remove_file_or_backing_child() on the other hand needs to ensure | ||
19 | that the indirect pointer it passes will stay valid for the duration of | ||
20 | the transaction. Ensure this by keeping a strong reference to the BDS | ||
21 | whose &bs->backing or &bs->file it passes to bdrv_replace_child_tran(), | ||
22 | and giving up that reference only in the transaction .clean() handler. | ||
23 | |||
24 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
25 | Message-Id: <20211111120829.81329-9-hreitz@redhat.com> | ||
26 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
27 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
28 | Message-Id: <20211115145409.176785-9-kwolf@redhat.com> | ||
29 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
30 | --- | ||
31 | block.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++------- | ||
32 | 1 file changed, 73 insertions(+), 10 deletions(-) | ||
33 | |||
34 | diff --git a/block.c b/block.c | ||
35 | index XXXXXXX..XXXXXXX 100644 | ||
36 | --- a/block.c | ||
37 | +++ b/block.c | ||
38 | @@ -XXX,XX +XXX,XX @@ static int bdrv_drv_set_perm(BlockDriverState *bs, uint64_t perm, | ||
39 | |||
40 | typedef struct BdrvReplaceChildState { | ||
41 | BdrvChild *child; | ||
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) | ||
128 | +{ | ||
129 | + BdrvRemoveFilterOrCowChild *s = opaque; | ||
130 | + | ||
131 | + /* Drop the bs reference after the transaction is done */ | ||
132 | + bdrv_unref(s->bs); | ||
133 | + g_free(s); | ||
134 | +} | ||
135 | + | ||
136 | static TransactionActionDrv bdrv_remove_filter_or_cow_child_drv = { | ||
137 | .abort = bdrv_remove_filter_or_cow_child_abort, | ||
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); | ||
231 | -- | ||
232 | 2.33.1 | ||
233 | |||
234 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | 1 | In most of the block layer, especially when traversing down from other | |
2 | BlockDriverStates, we assume that BdrvChild.bs can never be NULL. When | ||
3 | it becomes NULL, it is expected that the corresponding BdrvChild pointer | ||
4 | also becomes NULL and the BdrvChild object is freed. | ||
5 | |||
6 | Therefore, once bdrv_replace_child_noperm() sets the BdrvChild.bs | ||
7 | pointer to NULL, it should also immediately set the corresponding | ||
8 | BdrvChild pointer (like bs->file or bs->backing) to NULL. | ||
9 | |||
10 | In that context, it also makes sense for this function to free the | ||
11 | child. Sometimes we cannot do so, though, because it is called in a | ||
12 | transactional context where the caller might still want to reinstate the | ||
13 | child in the abort branch (and free it only on commit), so this behavior | ||
14 | has to remain optional. | ||
15 | |||
16 | In bdrv_replace_child_tran()'s abort handler, we now rely on the fact | ||
17 | that the BdrvChild passed to bdrv_replace_child_tran() must have had a | ||
18 | non-NULL .bs pointer initially. Make a note of that and assert it. | ||
19 | |||
20 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
21 | Message-Id: <20211111120829.81329-10-hreitz@redhat.com> | ||
22 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
23 | Message-Id: <20211115145409.176785-10-kwolf@redhat.com> | ||
24 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
25 | --- | ||
26 | block.c | 102 +++++++++++++++++++++++++++++++++++++++++++------------- | ||
27 | 1 file changed, 79 insertions(+), 23 deletions(-) | ||
28 | |||
29 | diff --git a/block.c b/block.c | ||
30 | index XXXXXXX..XXXXXXX 100644 | ||
31 | --- a/block.c | ||
32 | +++ b/block.c | ||
33 | @@ -XXX,XX +XXX,XX @@ static BlockDriverState *bdrv_open_inherit(const char *filename, | ||
34 | static bool bdrv_recurse_has_child(BlockDriverState *bs, | ||
35 | BlockDriverState *child); | ||
36 | |||
37 | +static void bdrv_child_free(BdrvChild *child); | ||
38 | static void bdrv_replace_child_noperm(BdrvChild **child, | ||
39 | - BlockDriverState *new_bs); | ||
40 | + BlockDriverState *new_bs, | ||
41 | + bool free_empty_child); | ||
42 | static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, | ||
43 | BdrvChild *child, | ||
44 | Transaction *tran); | ||
45 | @@ -XXX,XX +XXX,XX @@ typedef struct BdrvReplaceChildState { | ||
46 | BdrvChild *child; | ||
47 | BdrvChild **childp; | ||
48 | BlockDriverState *old_bs; | ||
49 | + bool free_empty_child; | ||
50 | } BdrvReplaceChildState; | ||
51 | |||
52 | static void bdrv_replace_child_commit(void *opaque) | ||
53 | { | ||
54 | BdrvReplaceChildState *s = opaque; | ||
55 | |||
56 | + if (s->free_empty_child && !s->child->bs) { | ||
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); | ||
131 | + | ||
132 | if (new_bs) { | ||
133 | bdrv_ref(new_bs); | ||
134 | } | ||
135 | - bdrv_replace_child_noperm(childp, new_bs); | ||
136 | + /* | ||
137 | + * Pass free_empty_child=false, we will free the child (if | ||
138 | + * necessary) in bdrv_replace_child_commit() (if our | ||
139 | + * @free_empty_child parameter was true). | ||
140 | + */ | ||
141 | + bdrv_replace_child_noperm(childp, new_bs, false); | ||
142 | /* old_bs reference is transparently moved from *childp to @s */ | ||
143 | } | ||
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 | } | ||
183 | + | ||
184 | + if (free_empty_child && !child->bs) { | ||
185 | + bdrv_child_free(child); | ||
186 | + } | ||
187 | } | ||
188 | |||
189 | /** | ||
190 | @@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque) | ||
191 | BdrvChild *child = *s->child; | ||
192 | BlockDriverState *bs = child->bs; | ||
193 | |||
194 | - bdrv_replace_child_noperm(s->child, NULL); | ||
195 | + /* | ||
196 | + * Pass free_empty_child=false, because we still need the child | ||
197 | + * for the AioContext operations on the parent below; those | ||
198 | + * BdrvChildClass methods all work on a BdrvChild object, so we | ||
199 | + * need to keep it as an empty shell (after this function, it will | ||
200 | + * not be attached to any parent, and it will not have a .bs). | ||
201 | + */ | ||
202 | + bdrv_replace_child_noperm(s->child, NULL, false); | ||
203 | |||
204 | if (bdrv_get_aio_context(bs) != s->old_child_ctx) { | ||
205 | bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort); | ||
206 | @@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque) | ||
207 | |||
208 | bdrv_unref(bs); | ||
209 | bdrv_child_free(child); | ||
210 | - *s->child = NULL; | ||
211 | } | ||
212 | |||
213 | static TransactionActionDrv bdrv_attach_child_common_drv = { | ||
214 | @@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_common(BlockDriverState *child_bs, | ||
215 | } | ||
216 | |||
217 | bdrv_ref(child_bs); | ||
218 | - bdrv_replace_child_noperm(&new_child, child_bs); | ||
219 | + bdrv_replace_child_noperm(&new_child, child_bs, true); | ||
220 | + /* child_bs was non-NULL, so new_child must not have been freed */ | ||
221 | + assert(new_child != NULL); | ||
222 | |||
223 | *child = new_child; | ||
224 | |||
225 | @@ -XXX,XX +XXX,XX @@ static void bdrv_detach_child(BdrvChild **childp) | ||
226 | { | ||
227 | BlockDriverState *old_bs = (*childp)->bs; | ||
228 | |||
229 | - bdrv_replace_child_noperm(childp, NULL); | ||
230 | - bdrv_child_free(*childp); | ||
231 | + bdrv_replace_child_noperm(childp, NULL, true); | ||
232 | |||
233 | if (old_bs) { | ||
234 | /* | ||
235 | @@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, | ||
236 | } | ||
237 | |||
238 | if (child->bs) { | ||
239 | - bdrv_replace_child_tran(childp, NULL, tran); | ||
240 | + /* | ||
241 | + * Pass free_empty_child=false, we will free the child in | ||
242 | + * bdrv_remove_filter_or_cow_child_commit() | ||
243 | + */ | ||
244 | + bdrv_replace_child_tran(childp, NULL, tran, false); | ||
245 | } | ||
246 | |||
247 | s = g_new(BdrvRemoveFilterOrCowChild, 1); | ||
248 | @@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, | ||
249 | .is_backing = (childp == &bs->backing), | ||
250 | }; | ||
251 | tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s); | ||
252 | - | ||
253 | - *childp = NULL; | ||
254 | } | ||
255 | |||
256 | /* | ||
257 | @@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from, | ||
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); | ||
277 | -- | ||
278 | 2.33.1 | ||
279 | |||
280 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | See the comment for why this is necessary. | ||
1 | 2 | ||
3 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
4 | Message-Id: <20211111120829.81329-11-hreitz@redhat.com> | ||
5 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
6 | Message-Id: <20211115145409.176785-11-kwolf@redhat.com> | ||
7 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
8 | --- | ||
9 | tests/qemu-iotests/030 | 11 ++++++++++- | ||
10 | 1 file changed, 10 insertions(+), 1 deletion(-) | ||
11 | |||
12 | diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 | ||
13 | index XXXXXXX..XXXXXXX 100755 | ||
14 | --- a/tests/qemu-iotests/030 | ||
15 | +++ b/tests/qemu-iotests/030 | ||
16 | @@ -XXX,XX +XXX,XX @@ class TestParallelOps(iotests.QMPTestCase): | ||
17 | speed=1024) | ||
18 | self.assert_qmp(result, 'return', {}) | ||
19 | |||
20 | - for job in pending_jobs: | ||
21 | + # Do this in reverse: After unthrottling them, some jobs may finish | ||
22 | + # before we have unthrottled all of them. This will drain their | ||
23 | + # subgraph, and this will make jobs above them advance (despite those | ||
24 | + # jobs on top being throttled). In the worst case, all jobs below the | ||
25 | + # top one are finished before we can unthrottle it, and this makes it | ||
26 | + # advance so far that it completes before we can unthrottle it - which | ||
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) | ||
32 | self.assert_qmp(result, 'return', {}) | ||
33 | |||
34 | -- | ||
35 | 2.33.1 | ||
36 | |||
37 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | From: Kevin Wolf <kwolf@redhat.com> | ||
1 | 2 | ||
3 | While introducing a non-QemuOpts code path for device creation for JSON | ||
4 | -device, we noticed that QMP device_add doesn't check its input | ||
5 | correctly (accepting arguments that should have been rejected), and that | ||
6 | users may be relying on this behaviour (libvirt did until it was fixed | ||
7 | recently). | ||
8 | |||
9 | Let's use a deprecation period before we fix this bug in QEMU to avoid | ||
10 | nasty surprises for users. | ||
11 | |||
12 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
13 | Message-Id: <20211111143530.18985-1-kwolf@redhat.com> | ||
14 | Reviewed-by: Markus Armbruster <armbru@redhat.com> | ||
15 | Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> | ||
16 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
17 | Message-Id: <20211115145409.176785-12-kwolf@redhat.com> | ||
18 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
19 | --- | ||
20 | docs/about/deprecated.rst | 14 ++++++++++++++ | ||
21 | 1 file changed, 14 insertions(+) | ||
22 | |||
23 | diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst | ||
24 | index XXXXXXX..XXXXXXX 100644 | ||
25 | --- a/docs/about/deprecated.rst | ||
26 | +++ b/docs/about/deprecated.rst | ||
27 | @@ -XXX,XX +XXX,XX @@ options are removed in favor of using explicit ``blockdev-create`` and | ||
28 | ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for | ||
29 | details. | ||
30 | |||
31 | +Incorrectly typed ``device_add`` arguments (since 6.2) | ||
32 | +'''''''''''''''''''''''''''''''''''''''''''''''''''''' | ||
33 | + | ||
34 | +Due to shortcomings in the internal implementation of ``device_add``, QEMU | ||
35 | +incorrectly accepts certain invalid arguments: Any object or list arguments are | ||
36 | +silently ignored. Other argument types are not checked, but an implicit | ||
37 | +conversion happens, so that e.g. string values can be assigned to integer | ||
38 | +device properties or vice versa. | ||
39 | + | ||
40 | +This is a bug in QEMU that will be fixed in the future so that previously | ||
41 | +accepted incorrect commands will return an error. Users should make sure that | ||
42 | +all arguments passed to ``device_add`` are consistent with the documented | ||
43 | +property types. | ||
44 | + | ||
45 | System accelerators | ||
46 | ------------------- | ||
47 | |||
48 | -- | ||
49 | 2.33.1 | ||
50 | |||
51 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | From: Stefan Hajnoczi <stefanha@redhat.com> | ||
1 | 2 | ||
3 | Reported by Coverity (CID 1465222). | ||
4 | |||
5 | Fixes: 4a1d937796de0fecd8b22d7dbebf87f38e8282fd ("softmmu/qdev-monitor: add error handling in qdev_set_id") | ||
6 | Cc: Damien Hedde <damien.hedde@greensocs.com> | ||
7 | Cc: Kevin Wolf <kwolf@redhat.com> | ||
8 | Cc: Michael S. Tsirkin <mst@redhat.com> | ||
9 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
10 | Message-Id: <20211102163342.31162-1-stefanha@redhat.com> | ||
11 | Reviewed-by: Kevin Wolf <kwolf@redhat.com> | ||
12 | Reviewed-by: Michael S. Tsirkin <mst@redhat.com> | ||
13 | Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> | ||
14 | Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> | ||
15 | Reviewed-by: Damien Hedde <damien.hedde@greensocs.com> | ||
16 | Reviewed-by: Markus Armbruster <armbru@redhat.com> | ||
17 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> | ||
18 | Message-Id: <20211115145409.176785-14-kwolf@redhat.com> | ||
19 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
20 | --- | ||
21 | softmmu/qdev-monitor.c | 2 +- | ||
22 | 1 file changed, 1 insertion(+), 1 deletion(-) | ||
23 | |||
24 | diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c | ||
25 | index XXXXXXX..XXXXXXX 100644 | ||
26 | --- a/softmmu/qdev-monitor.c | ||
27 | +++ b/softmmu/qdev-monitor.c | ||
28 | @@ -XXX,XX +XXX,XX @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) | ||
29 | if (prop) { | ||
30 | dev->id = id; | ||
31 | } else { | ||
32 | - g_free(id); | ||
33 | error_setg(errp, "Duplicate device ID '%s'", id); | ||
34 | + g_free(id); | ||
35 | return NULL; | ||
36 | } | ||
37 | } else { | ||
38 | -- | ||
39 | 2.33.1 | ||
40 | |||
41 | diff view generated by jsdifflib |
1 | From: Peter Maydell <peter.maydell@linaro.org> | 1 | From: Kevin Wolf <kwolf@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | In qemu_gluster_parse_json(), the call to qdict_array_entries() | 3 | At the end of a reopen, we already call bdrv_refresh_limits(), which |
4 | could return a negative error code, which we were ignoring | 4 | should update bs->request_alignment according to the new file |
5 | because we assigned the result to an unsigned variable. | 5 | descriptor. However, raw_probe_alignment() relies on s->needs_alignment |
6 | Fix this by using the 'int' type instead, which matches the | 6 | and just uses 1 if it isn't set. We neglected to update this field, so |
7 | return type of qdict_array_entries() and also the type | 7 | starting with cache=writeback and then reopening with cache=none means |
8 | we use for the loop enumeration variable 'i'. | 8 | that we get an incorrect bs->request_alignment == 1 and unaligned |
9 | requests fail instead of being automatically aligned. | ||
9 | 10 | ||
10 | (Spotted by Coverity, CID 1360960.) | 11 | Fix this by recalculating s->needs_alignment in raw_refresh_limits() |
12 | before calling raw_probe_alignment(). | ||
11 | 13 | ||
12 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 14 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
13 | Reviewed-by: Eric Blake <eblake@redhat.com> | 15 | Message-Id: <20211104113109.56336-1-kwolf@redhat.com> |
14 | Reviewed-by: Jeff Cody <jcody@redhat.com> | 16 | Reviewed-by: Hanna Reitz <hreitz@redhat.com> |
15 | Message-id: 1496682098-1540-1-git-send-email-peter.maydell@linaro.org | 17 | Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> |
16 | Signed-off-by: Jeff Cody <jcody@redhat.com> | 18 | Signed-off-by: Kevin Wolf <kwolf@redhat.com> |
19 | Message-Id: <20211115145409.176785-13-kwolf@redhat.com> | ||
20 | [hreitz: Fix iotest 142 for block sizes greater than 512 by operating on | ||
21 | a file with a size of 1 MB] | ||
22 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
23 | Message-Id: <20211116101431.105252-1-hreitz@redhat.com> | ||
17 | --- | 24 | --- |
18 | block/gluster.c | 3 +-- | 25 | block/file-posix.c | 20 ++++++++++++++++---- |
19 | 1 file changed, 1 insertion(+), 2 deletions(-) | 26 | tests/qemu-iotests/142 | 29 +++++++++++++++++++++++++++++ |
27 | tests/qemu-iotests/142.out | 18 ++++++++++++++++++ | ||
28 | 3 files changed, 63 insertions(+), 4 deletions(-) | ||
20 | 29 | ||
21 | diff --git a/block/gluster.c b/block/gluster.c | 30 | diff --git a/block/file-posix.c b/block/file-posix.c |
22 | index XXXXXXX..XXXXXXX 100644 | 31 | index XXXXXXX..XXXXXXX 100644 |
23 | --- a/block/gluster.c | 32 | --- a/block/file-posix.c |
24 | +++ b/block/gluster.c | 33 | +++ b/block/file-posix.c |
25 | @@ -XXX,XX +XXX,XX @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf, | 34 | @@ -XXX,XX +XXX,XX @@ typedef struct BDRVRawState { |
26 | Error *local_err = NULL; | 35 | int page_cache_inconsistent; /* errno from fdatasync failure */ |
27 | char *str = NULL; | 36 | bool has_fallocate; |
28 | const char *ptr; | 37 | bool needs_alignment; |
29 | - size_t num_servers; | 38 | + bool force_alignment; |
30 | - int i, type; | 39 | bool drop_cache; |
31 | + int i, type, num_servers; | 40 | bool check_cache_dropped; |
32 | 41 | struct { | |
33 | /* create opts info from runtime_json_opts list */ | 42 | @@ -XXX,XX +XXX,XX @@ static bool dio_byte_aligned(int fd) |
34 | opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort); | 43 | return false; |
44 | } | ||
45 | |||
46 | +static bool raw_needs_alignment(BlockDriverState *bs) | ||
47 | +{ | ||
48 | + BDRVRawState *s = bs->opaque; | ||
49 | + | ||
50 | + if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) { | ||
51 | + return true; | ||
52 | + } | ||
53 | + | ||
54 | + return s->force_alignment; | ||
55 | +} | ||
56 | + | ||
57 | /* Check if read is allowed with given memory buffer and length. | ||
58 | * | ||
59 | * This function is used to check O_DIRECT memory buffer and request alignment. | ||
60 | @@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options, | ||
61 | |||
62 | s->has_discard = true; | ||
63 | s->has_write_zeroes = true; | ||
64 | - if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) { | ||
65 | - s->needs_alignment = true; | ||
66 | - } | ||
67 | |||
68 | if (fstat(s->fd, &st) < 0) { | ||
69 | ret = -errno; | ||
70 | @@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options, | ||
71 | * so QEMU makes sure all IO operations on the device are aligned | ||
72 | * to sector size, or else FreeBSD will reject them with EINVAL. | ||
73 | */ | ||
74 | - s->needs_alignment = true; | ||
75 | + s->force_alignment = true; | ||
76 | } | ||
77 | #endif | ||
78 | + s->needs_alignment = raw_needs_alignment(bs); | ||
79 | |||
80 | #ifdef CONFIG_XFS | ||
81 | if (platform_test_xfs_fd(s->fd)) { | ||
82 | @@ -XXX,XX +XXX,XX @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) | ||
83 | BDRVRawState *s = bs->opaque; | ||
84 | struct stat st; | ||
85 | |||
86 | + s->needs_alignment = raw_needs_alignment(bs); | ||
87 | raw_probe_alignment(bs, s->fd, errp); | ||
88 | + | ||
89 | bs->bl.min_mem_alignment = s->buf_align; | ||
90 | bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size); | ||
91 | |||
92 | diff --git a/tests/qemu-iotests/142 b/tests/qemu-iotests/142 | ||
93 | index XXXXXXX..XXXXXXX 100755 | ||
94 | --- a/tests/qemu-iotests/142 | ||
95 | +++ b/tests/qemu-iotests/142 | ||
96 | @@ -XXX,XX +XXX,XX @@ info block backing-file" | ||
97 | |||
98 | echo "$hmp_cmds" | run_qemu -drive "$files","$ids" | grep "Cache" | ||
99 | |||
100 | +echo | ||
101 | +echo "--- Alignment after changing O_DIRECT ---" | ||
102 | +echo | ||
103 | + | ||
104 | +# Directly test the protocol level: Can unaligned requests succeed even if | ||
105 | +# O_DIRECT was only enabled through a reopen and vice versa? | ||
106 | + | ||
107 | +# Ensure image size is a multiple of the sector size (required for O_DIRECT) | ||
108 | +$QEMU_IMG create -f file "$TEST_IMG" 1M | _filter_img_create | ||
109 | + | ||
110 | +# And write some data (not strictly necessary, but it feels better to actually | ||
111 | +# have something to be read) | ||
112 | +$QEMU_IO -f file -c 'write 0 4096' "$TEST_IMG" | _filter_qemu_io | ||
113 | + | ||
114 | +$QEMU_IO --cache=writeback -f file $TEST_IMG <<EOF | _filter_qemu_io | ||
115 | +read 42 42 | ||
116 | +reopen -o cache.direct=on | ||
117 | +read 42 42 | ||
118 | +reopen -o cache.direct=off | ||
119 | +read 42 42 | ||
120 | +EOF | ||
121 | +$QEMU_IO --cache=none -f file $TEST_IMG <<EOF | _filter_qemu_io | ||
122 | +read 42 42 | ||
123 | +reopen -o cache.direct=off | ||
124 | +read 42 42 | ||
125 | +reopen -o cache.direct=on | ||
126 | +read 42 42 | ||
127 | +EOF | ||
128 | + | ||
129 | # success, all done | ||
130 | echo "*** done" | ||
131 | rm -f $seq.full | ||
132 | diff --git a/tests/qemu-iotests/142.out b/tests/qemu-iotests/142.out | ||
133 | index XXXXXXX..XXXXXXX 100644 | ||
134 | --- a/tests/qemu-iotests/142.out | ||
135 | +++ b/tests/qemu-iotests/142.out | ||
136 | @@ -XXX,XX +XXX,XX @@ cache.no-flush=on on backing-file | ||
137 | Cache mode: writeback | ||
138 | Cache mode: writeback, direct | ||
139 | Cache mode: writeback, ignore flushes | ||
140 | + | ||
141 | +--- Alignment after changing O_DIRECT --- | ||
142 | + | ||
143 | +Formatting 'TEST_DIR/t.IMGFMT', fmt=file size=1048576 | ||
144 | +wrote 4096/4096 bytes at offset 0 | ||
145 | +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) | ||
146 | +read 42/42 bytes at offset 42 | ||
147 | +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) | ||
148 | +read 42/42 bytes at offset 42 | ||
149 | +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) | ||
150 | +read 42/42 bytes at offset 42 | ||
151 | +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) | ||
152 | +read 42/42 bytes at offset 42 | ||
153 | +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) | ||
154 | +read 42/42 bytes at offset 42 | ||
155 | +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) | ||
156 | +read 42/42 bytes at offset 42 | ||
157 | +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) | ||
158 | *** done | ||
35 | -- | 159 | -- |
36 | 2.9.3 | 160 | 2.33.1 |
37 | 161 | ||
38 | 162 | diff view generated by jsdifflib |