| Message ID | 20190510211119.29376-1-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blockdev: loosen restrictions on drive-backup source node | expand |
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. 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) >
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. > > 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) >> >
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.
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; + } 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)
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(-)