[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 2/7] block: swap operation order in bdrv_appe
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v8 2/7] block: swap operation order in bdrv_append |
Date: |
Thu, 13 Jun 2019 15:45:28 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote:
> bs_top parents may conflict with bs_new backing child permissions, so
> let's do bdrv_replace_node first, it covers more possible cases.
>
> It is needed for further implementation of backup-top filter, which
> don't want to share write permission on its backing child.
>
> Side effect is that we may set backing hd when device name is already
> available, so 085 iotest output is changed.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
> block.c | 11 ++++++++---
> tests/qemu-iotests/085.out | 2 +-
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index e6e9770704..57216f4115 100644
> --- a/block.c
> +++ b/block.c
> @@ -4088,22 +4088,27 @@ void bdrv_append(BlockDriverState *bs_new,
> BlockDriverState *bs_top,
> {
> Error *local_err = NULL;
>
> - bdrv_set_backing_hd(bs_new, bs_top, &local_err);
> + bdrv_ref(bs_top);
> +
> + bdrv_replace_node(bs_top, bs_new, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> + error_prepend(errp, "Failed to replace node: ");
> goto out;
> }
>
> - bdrv_replace_node(bs_top, bs_new, &local_err);
> + bdrv_set_backing_hd(bs_new, bs_top, &local_err);
> if (local_err) {
> + bdrv_replace_node(bs_new, bs_top, &error_abort);
Hm. I see the need for switching this stuff around, but this
&error_abort is much more difficult to verify than the previous one for
bdrv_set_backing_hd(..., NULL, ...). I think it at least warrants a
comment why the order has to be this way (using git blame has the
disadvantage of fading over time as other people modify a piece of
code), and why this &error_abort is safe.
Max
> error_propagate(errp, local_err);
> - bdrv_set_backing_hd(bs_new, NULL, &error_abort);
> + error_prepend(errp, "Failed to set backing: ");
> goto out;
> }
>
> /* bs_new is now referenced by its new parents, we don't need the
> * additional reference any more. */
> out:
> + bdrv_unref(bs_top);
> bdrv_unref(bs_new);
> }
>
> diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
> index 6edf107f55..e5a2645bf5 100644
> --- a/tests/qemu-iotests/085.out
> +++ b/tests/qemu-iotests/085.out
> @@ -74,7 +74,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> backing_file=TEST_DIR/
>
> === Invalid command - snapshot node used as backing hd ===
>
> -{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node is
> used as backing hd of 'snap_12'"}}
> +{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node is
> used as backing hd of 'virtio0'"}}
>
> === Invalid command - snapshot node has a backing image ===
>
>
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v8 2/7] block: swap operation order in bdrv_append,
Max Reitz <=