Re: [Qemu-devel] [PATCH] blockdev: loosen restrictions on drive-backup source node

[John Snow <jsnow@redhat.com>]

On 5/21/19 5:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> 21.05.2019 1:38, John Snow wrote:
>>
>>
>> On 5/10/19 5:11 PM, John Snow wrote:
>>> We mandate that the source node must be a root node; but there's no reason
>>> I am aware of that it needs to be restricted to such. In some cases, we need
>>> to make sure that there's a medium present, but in the general case we can
>>> allow the backup job itself to do the graph checking.
>>>
>>> This patch helps improve the error message when you try to backup from
>>> the same node more than once, which is reflected in the change to test
>>> 056.
>>>
>>> For backups with bitmaps, it will also show a better error message that
>>> the bitmap is in use instead of giving you something cryptic like "need
>>> a root node."
>>>
>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1707303
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   blockdev.c             | 6 +++++-
>>>   tests/qemu-iotests/056 | 2 +-
>>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 79fbac8450..27cb72f7aa 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -3450,7 +3450,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
>>>           backup->compress = false;
>>>       }
>>>   
>>> -    bs = qmp_get_root_bs(backup->device, errp);
>>> +    bs = bdrv_lookup_bs(backup->device, backup->device, errp);
>>>       if (!bs) {
>>>           return NULL;
>>>       }
>>> @@ -3459,6 +3459,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
>>>       aio_context_acquire(aio_context);
>>>   
>>>       if (!backup->has_format) {
>>> +        if (!bs->drv) {
>>> +            error_setg(errp, "Device has no medium");
>>> +            return NULL;
>>> +        }
>>
>> Pinging my own patch with a review comment. It is weird that I shuffled
>> the error checking down below a conditional, but it's the only case
>> where we directly need do access bs->drv now.
> 
> not the only: it's accessed in following
>      if (backup->mode != NEW_IMAGE_MODE_EXISTING) {
>          assert(backup->format);
>          if (source) {
> as well.
> 

Ah, of course. Thanks.