[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v8 2/7] block: swap operation order in bdrv_appe
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH v8 2/7] block: swap operation order in bdrv_append |
Date: |
Thu, 13 Jun 2019 14:02:37 +0000 |
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 <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
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