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 <vsement...@virtuozzo.com> > --- > 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