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

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
>> ---
>>   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.

> 
> Otherwise, block/backup already checks for this in its own routine and I
> felt like it was best to let the job handle if it had the right type of
> arguments instead of splitting that out up here.
> 
> Still, it probably looks weird to see the "Device has no medium" error
> in a conditional here, so if this patch looks okay otherwise, I can send
> a v2 with that error checking shuffled back up to top-level to maintain
> some consistency with how the error checking used to be handled.
> 
>>           backup->format = backup->mode == NEW_IMAGE_MODE_EXISTING ?
>>                            NULL : (char*) bs->drv->format_name;
>>       }
>> diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
>> index 3df323984d..f40fc11a09 100755
>> --- a/tests/qemu-iotests/056
>> +++ b/tests/qemu-iotests/056
>> @@ -214,7 +214,7 @@ class BackupTest(iotests.QMPTestCase):
>>           res = self.vm.qmp('query-block-jobs')
>>           self.assert_qmp(res, 'return[0]/status', 'concluded')
>>           # Leave zombie job un-dismissed, observe a failure:
>> -        res = self.qmp_backup_and_wait(serror='Need a root block node',
>> +        res = self.qmp_backup_and_wait(serror="Node 'drive0' is busy: block 
>> device is in use by block job: backup",
>>                                          device='drive0', 
>> format=iotests.imgfmt,
>>                                          sync='full', target=self.dest_img,
>>                                          auto_dismiss=False)
>>
> 


-- 
Best regards,
Vladimir