Re: [Qemu-devel] [PATCH v8 2/7] block: swap operation order in bdrv_append

[Max Reitz]

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