diff mbox

[3/3] block: use bdrv_lookup_bs() over blk_by_name() for BDS only results

Message ID d61ea8e979792a8a202b81f33c160cde123abbc6.1444827043.git.jcody@redhat.com
State New
Headers show

Commit Message

Jeff Cody Oct. 14, 2015, 1:16 p.m. UTC 
To find a BlockDriverState interface, it can be done via blk_by_name(),
bdrv_find_node(), and bdrv_lookup_bs().  The latter can take the place
of the other two, in the instances where we are only concerned with the
BlockDriverState.

In much of the usage of blk_by_name(), we can simplify the code by
calling bdrv_lookup_bs() instead of blk_by_name(), when what we need is
just the BDS, and not the BB.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 blockdev.c        | 84 ++++++++++++++++++++-----------------------------------
 migration/block.c |  6 ++--
 2 files changed, 32 insertions(+), 58 deletions(-)

Comments

Max Reitz Oct. 14, 2015, 6:05 p.m. UTC | #1 
On 14.10.2015 15:16, Jeff Cody wrote:
> To find a BlockDriverState interface, it can be done via blk_by_name(),
> bdrv_find_node(), and bdrv_lookup_bs().  The latter can take the place
> of the other two, in the instances where we are only concerned with the
> BlockDriverState.
> 
> In much of the usage of blk_by_name(), we can simplify the code by
> calling bdrv_lookup_bs() instead of blk_by_name(), when what we need is
> just the BDS, and not the BB.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  blockdev.c        | 84 ++++++++++++++++++++-----------------------------------
>  migration/block.c |  6 ++--
>  2 files changed, 32 insertions(+), 58 deletions(-)

Again, if this series is based on Berto's blockdev-snapshot series, it
should be based on my BB series as well. But with that series applied,
this patch has some (non-trivial) rebase conflicts on it.

Also, it has a fundamental conflict with that series: If a BB can be
found by bdrv_lookup_bs() but it doesn't have a BDS, it will return NULL
and set *errp accordingly ("No medium in device"). However, this patch
discards that error message and just keeps the one of the former
blk_by_name() caller, which is generally "Device not found". That is
wrong, however, if there is simply no medium in the device.

In some cases, that doesn't matter (since we just assume that if there
is a BB, it will have a BDS, like for hmp_commit), but it most probably
does matter for all the places which conflict with my BB series, the
reason being that they generally conflict because I added a
blk_is_available() check.

Note that this is a "design conflict", too: bdrv_lookup_bs() will return
NULL only if blk_bs() is NULL. blk_is_available() may return false even
if there is a BDS (if the BDS is unavailable, e.g. one of the BDSs in
the tree not being inserted or the guest device's tray is open). But if
you already have a BDS (because you used bdrv_lookup_bs()), calling
blk_is_available() on bs->blk afterwards looks kind of strange...

Max
Jeff Cody Oct. 14, 2015, 6:13 p.m. UTC | #2 
On Wed, Oct 14, 2015 at 08:05:34PM +0200, Max Reitz wrote:
> On 14.10.2015 15:16, Jeff Cody wrote:
> > To find a BlockDriverState interface, it can be done via blk_by_name(),
> > bdrv_find_node(), and bdrv_lookup_bs().  The latter can take the place
> > of the other two, in the instances where we are only concerned with the
> > BlockDriverState.
> > 
> > In much of the usage of blk_by_name(), we can simplify the code by
> > calling bdrv_lookup_bs() instead of blk_by_name(), when what we need is
> > just the BDS, and not the BB.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  blockdev.c        | 84 ++++++++++++++++++++-----------------------------------
> >  migration/block.c |  6 ++--
> >  2 files changed, 32 insertions(+), 58 deletions(-)
> 
> Again, if this series is based on Berto's blockdev-snapshot series, it
> should be based on my BB series as well. But with that series applied,
> this patch has some (non-trivial) rebase conflicts on it.
> 
> Also, it has a fundamental conflict with that series: If a BB can be
> found by bdrv_lookup_bs() but it doesn't have a BDS, it will return NULL
> and set *errp accordingly ("No medium in device"). However, this patch
> discards that error message and just keeps the one of the former
> blk_by_name() caller, which is generally "Device not found". That is
> wrong, however, if there is simply no medium in the device.
>

Good point.  We can just drop this patch, then - maybe at some point,
we can have a consolidated API to obtain a BDS (if having such a thing
is even all that important), that fits well with the design goal of
your series.

I think the first two patches are probably worth keeping, however.

> In some cases, that doesn't matter (since we just assume that if there
> is a BB, it will have a BDS, like for hmp_commit), but it most probably
> does matter for all the places which conflict with my BB series, the
> reason being that they generally conflict because I added a
> blk_is_available() check.
> 
> Note that this is a "design conflict", too: bdrv_lookup_bs() will return
> NULL only if blk_bs() is NULL. blk_is_available() may return false even
> if there is a BDS (if the BDS is unavailable, e.g. one of the BDSs in
> the tree not being inserted or the guest device's tray is open). But if
> you already have a BDS (because you used bdrv_lookup_bs()), calling
> blk_is_available() on bs->blk afterwards looks kind of strange...
> 
> Max
>
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index fe182bb..6a893e1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1033,18 +1033,18 @@  fail:
 void hmp_commit(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
-    BlockBackend *blk;
+    BlockDriverState *bs;
     int ret;
 
     if (!strcmp(device, "all")) {
         ret = bdrv_commit_all();
     } else {
-        blk = blk_by_name(device);
-        if (!blk) {
+        bs = bdrv_lookup_bs(device, NULL, NULL);
+        if (!bs) {
             monitor_printf(mon, "Device '%s' not found\n", device);
             return;
         }
-        ret = bdrv_commit(blk_bs(blk));
+        ret = bdrv_commit(bs);
     }
     if (ret < 0) {
         monitor_printf(mon, "'commit' error for '%s': %s\n", device,
@@ -1122,20 +1122,18 @@  SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
                                                          Error **errp)
 {
     BlockDriverState *bs;
-    BlockBackend *blk;
     AioContext *aio_context;
     QEMUSnapshotInfo sn;
     Error *local_err = NULL;
     SnapshotInfo *info = NULL;
     int ret;
 
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device, NULL, NULL);
+    if (!bs) {
         error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", device);
         return NULL;
     }
-    bs = blk_bs(blk);
 
     if (!has_id) {
         id = NULL;
@@ -1300,7 +1298,6 @@  static void internal_snapshot_prepare(BlkTransactionState *common,
     Error *local_err = NULL;
     const char *device;
     const char *name;
-    BlockBackend *blk;
     BlockDriverState *bs;
     QEMUSnapshotInfo old_sn, *sn;
     bool ret;
@@ -1319,13 +1316,12 @@  static void internal_snapshot_prepare(BlkTransactionState *common,
     name = internal->name;
 
     /* 2. check for validation */
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device, NULL, NULL);
+    if (!bs) {
         error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", device);
         return;
     }
-    bs = blk_bs(blk);
 
     /* AioContext is released in .clean() */
     state->aio_context = bdrv_get_aio_context(bs);
@@ -1613,20 +1609,18 @@  static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
     BlockDriverState *bs;
-    BlockBackend *blk;
     DriveBackup *backup;
     Error *local_err = NULL;
 
     assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
     backup = common->action->drive_backup;
 
-    blk = blk_by_name(backup->device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(backup->device, NULL, NULL);
+    if (!bs) {
         error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", backup->device);
         return;
     }
-    bs = blk_bs(blk);
 
     /* AioContext is released in .clean() */
     state->aio_context = bdrv_get_aio_context(bs);
@@ -1682,25 +1676,22 @@  static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
     BlockdevBackup *backup;
     BlockDriverState *bs, *target;
-    BlockBackend *blk;
     Error *local_err = NULL;
 
     assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
     backup = common->action->blockdev_backup;
 
-    blk = blk_by_name(backup->device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(backup->device, NULL, NULL);
+    if (!bs) {
         error_setg(errp, "Device '%s' not found", backup->device);
         return;
     }
-    bs = blk_bs(blk);
 
-    blk = blk_by_name(backup->target);
-    if (!blk) {
+    target = bdrv_lookup_bs(backup->target, NULL, NULL);
+    if (!target) {
         error_setg(errp, "Device '%s' not found", backup->target);
         return;
     }
-    target = blk_bs(blk);
 
     /* AioContext is released in .clean() */
     state->aio_context = bdrv_get_aio_context(bs);
@@ -2324,7 +2315,6 @@  void qmp_block_stream(const char *device,
                       bool has_on_error, BlockdevOnError on_error,
                       Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *base_bs = NULL;
     AioContext *aio_context;
@@ -2335,13 +2325,12 @@  void qmp_block_stream(const char *device,
         on_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device, NULL, NULL);
+    if (!bs) {
         error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", device);
         return;
     }
-    bs = blk_bs(blk);
 
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
@@ -2391,7 +2380,6 @@  void qmp_block_commit(const char *device,
                       bool has_speed, int64_t speed,
                       Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *base_bs, *top_bs;
     AioContext *aio_context;
@@ -2410,13 +2398,12 @@  void qmp_block_commit(const char *device,
      *  live commit feature versions; for this to work, we must make sure to
      *  perform the device lookup before any generic errors that may occur in a
      *  scenario in which all optional arguments are omitted. */
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device, NULL, NULL);
+    if (!bs) {
         error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", device);
         return;
     }
-    bs = blk_bs(blk);
 
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
@@ -2495,7 +2482,6 @@  void qmp_drive_backup(const char *device, const char *target,
                       bool has_on_target_error, BlockdevOnError on_target_error,
                       Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *target_bs;
     BlockDriverState *source = NULL;
@@ -2520,13 +2506,12 @@  void qmp_drive_backup(const char *device, const char *target,
         mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device, NULL, NULL);
+    if (!bs) {
         error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", device);
         return;
     }
-    bs = blk_bs(blk);
 
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
@@ -2633,7 +2618,6 @@  void qmp_blockdev_backup(const char *device, const char *target,
                          BlockdevOnError on_target_error,
                          Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *target_bs;
     Error *local_err = NULL;
@@ -2649,22 +2633,20 @@  void qmp_blockdev_backup(const char *device, const char *target,
         on_target_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device, NULL, NULL);
+    if (!bs) {
         error_setg(errp, "Device '%s' not found", device);
         return;
     }
-    bs = blk_bs(blk);
 
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    blk = blk_by_name(target);
-    if (!blk) {
+    target_bs = bdrv_lookup_bs(target, NULL, NULL);
+    if (!target_bs) {
         error_setg(errp, "Device '%s' not found", target);
         goto out;
     }
-    target_bs = blk_bs(blk);
 
     bdrv_ref(target_bs);
     bdrv_set_aio_context(target_bs, aio_context);
@@ -2692,7 +2674,6 @@  void qmp_drive_mirror(const char *device, const char *target,
                       bool has_unmap, bool unmap,
                       Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *source, *target_bs;
     AioContext *aio_context;
@@ -2735,13 +2716,12 @@  void qmp_drive_mirror(const char *device, const char *target,
         return;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device, NULL, NULL);
+    if (!bs) {
         error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", device);
         return;
     }
-    bs = blk_bs(blk);
 
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
@@ -2876,14 +2856,12 @@  out:
 static BlockJob *find_block_job(const char *device, AioContext **aio_context,
                                 Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
 
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device, NULL, NULL);
+    if (!bs) {
         goto notfound;
     }
-    bs = blk_bs(blk);
 
     *aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(*aio_context);
@@ -2990,7 +2968,6 @@  void qmp_change_backing_file(const char *device,
                              const char *backing_file,
                              Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs = NULL;
     AioContext *aio_context;
     BlockDriverState *image_bs = NULL;
@@ -2999,13 +2976,12 @@  void qmp_change_backing_file(const char *device,
     int open_flags;
     int ret;
 
-    blk = blk_by_name(device);
-    if (!blk) {
+    bs = bdrv_lookup_bs(device, NULL, NULL);
+    if (!bs) {
         error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", device);
         return;
     }
-    bs = blk_bs(blk);
 
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
diff --git a/migration/block.c b/migration/block.c
index ed865ed..6b24be8 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -783,7 +783,6 @@  static int block_load(QEMUFile *f, void *opaque, int version_id)
     char device_name[256];
     int64_t addr;
     BlockDriverState *bs, *bs_prev = NULL;
-    BlockBackend *blk;
     uint8_t *buf;
     int64_t total_sectors = 0;
     int nr_sectors;
@@ -801,13 +800,12 @@  static int block_load(QEMUFile *f, void *opaque, int version_id)
             qemu_get_buffer(f, (uint8_t *)device_name, len);
             device_name[len] = '\0';
 
-            blk = blk_by_name(device_name);
-            if (!blk) {
+            bs = bdrv_lookup_bs(device_name, NULL, NULL);
+            if (!bs) {
                 fprintf(stderr, "Error unknown block device %s\n",
                         device_name);
                 return -EINVAL;
             }
-            bs = blk_bs(blk);
 
             if (bs != bs_prev) {
                 bs_prev = bs;