| Message ID | 1422387983-32153-20-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/27/2015 12:45 PM, Max Reitz wrote: > When preparing a blockdev-backup transaction, the BlockBackend should be > used because there may be no medium associated to the BB (which would > make bdrv_find() fail, whereas blk_by_name() does not). > > This does not make a real difference because blockdev-backup will fail > without a medium anyway; however, it will have an impact on the error > returned ("device not found" vs. "no medium"). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > blockdev.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) And no tests changed output as a result? Might be worth adding a test for the error message. Reviewed-by: Eric Blake <eblake@redhat.com>
On 2015-01-27 at 16:59, Eric Blake wrote: > On 01/27/2015 12:45 PM, Max Reitz wrote: >> When preparing a blockdev-backup transaction, the BlockBackend should be >> used because there may be no medium associated to the BB (which would >> make bdrv_find() fail, whereas blk_by_name() does not). >> >> This does not make a real difference because blockdev-backup will fail >> without a medium anyway; however, it will have an impact on the error >> returned ("device not found" vs. "no medium"). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> blockdev.c | 17 ++++++++++------- >> 1 file changed, 10 insertions(+), 7 deletions(-) > And no tests changed output as a result? Might be worth adding a test > for the error message. Actually, it's preventing a test output change: At this point, a BB always has a BDS, thus there is no change (if blk_by_name() works, bdrv_find() will work, too). This will change only later (patches 34 and 40), at which point the message would change to "device not found" without this patch (and as far as I remember, qemu-iotest 055 tests the error type). Max
diff --git a/blockdev.c b/blockdev.c index c9e9ab9..ae1137f 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1562,27 +1562,27 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp) { BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); BlockdevBackup *backup; - BlockDriverState *bs, *target; + BlockBackend *blk, *target; Error *local_err = NULL; assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP); backup = common->action->blockdev_backup; - bs = bdrv_find(backup->device); - if (!bs) { + blk = blk_by_name(backup->device); + if (!blk) { error_set(errp, QERR_DEVICE_NOT_FOUND, backup->device); return; } - target = bdrv_find(backup->target); + target = blk_by_name(backup->target); if (!target) { error_set(errp, QERR_DEVICE_NOT_FOUND, backup->target); return; } /* AioContext is released in .clean() */ - state->aio_context = bdrv_get_aio_context(bs); - if (state->aio_context != bdrv_get_aio_context(target)) { + state->aio_context = blk_get_aio_context(blk); + if (state->aio_context != blk_get_aio_context(target)) { state->aio_context = NULL; error_setg(errp, "Backup between two IO threads is not implemented"); return; @@ -1600,7 +1600,10 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp) return; } - state->bs = bs; + /* qmp_blockdev_backup() would have failed otherwise */ + assert(blk_bs(blk)); + + state->bs = blk_bs(blk); state->job = state->bs->job; }
When preparing a blockdev-backup transaction, the BlockBackend should be used because there may be no medium associated to the BB (which would make bdrv_find() fail, whereas blk_by_name() does not). This does not make a real difference because blockdev-backup will fail without a medium anyway; however, it will have an impact on the error returned ("device not found" vs. "no medium"). Signed-off-by: Max Reitz <mreitz@redhat.com> --- blockdev.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)