13.06.2019 16:45, Max Reitz wrote: > 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
so, I always use git log -p -- <filepath> instead, and search through it) > code), and why this &error_abort is safe. ok, will add > > 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 === >> >> > > -- Best regards, Vladimir