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 <[email protected]> > --- > 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
