| Message ID | 20190529154654.95870-3-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | backup-top filter driver for backup | expand |
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 <vsementsov@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 === > >
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 <vsementsov@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 === >> >> > >
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); 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 ===
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 <vsementsov@virtuozzo.com> --- block.c | 11 ++++++++--- tests/qemu-iotests/085.out | 2 +- 2 files changed, 9 insertions(+), 4 deletions(-)