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 <js...@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.
>
> 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
