Re: [Qemu-devel] [PATCH v8 2/7] block: swap operation order in bdrv_append

[Vladimir Sementsov-Ogievskiy]

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