Prefer bdrv_lookup_bs() to find BDS nodes

PATCH In review Jeff Cody 4 participants 13 messages Oct 14, 2015 – Oct 16, 2015 Block layer

lore

Share & tools

lore

Jeff Cody

author

2015-10-14 13:16 UTC #0 lore raw

cover Prefer bdrv_lookup_bs() to find BDS nodes

This series is on top of Alberto's "Add 'blockdev-snapshot' command" series

BlockBackend devices and BlockDriverState node-names occupy the same namespace. In addition, there are two methods that can be used in different circumstances to look up a BlockDriverState: blk_by_name() and bdrv_find_node().

There is also a common interface, bdrv_lookup_bs(), that can look up either by blk_by_name() or bdrv_find_node().

This makes bdrv_find_node() redundant for an external interface. And in the cases where we just want the underlying BDS for a BlockBackend device, it is simpler to use bdrv_lookup_bs() instead of blk_by_name().

This series makes bdrv_find_node() static and internal to block.c as a helper function, and attempts to simplify some code when we are looking just for the BDS of a BlockBackend device.

Jeff Cody (3):
block: Use bdrv_lookup_bs() instead of bdrv_find_node()
block: make bdrv_find_node() static
block: use bdrv_lookup_bs() over blk_by_name() for BDS only results

 block.c                 | 30 +++++++++---------
 block/block-backend.c   |  2 +-
 block/mirror.c          |  2 +-
 block/write-threshold.c |  2 +-
 blockdev.c              | 84 ++++++++++++++++++-------------------------------
 include/block/block.h   |  1 -
 migration/block.c       |  6 ++--
 7 files changed, 50 insertions(+), 77 deletions(-)

Signature · 1 line

1.9.3

Jeff Cody

author

2015-10-14 13:16 UTC #7 lore raw

PATCH 2/3 block: make bdrv_find_node() static

This patch does two things: it moves bdrv_find_node() up before the first usage in block.c, and it makes the function static so that it is only internal to block.c.

Signed-off-by Jeff Cody <[email protected]>

---
 block.c               | 30 +++++++++++++++---------------
 include/block/block.h |  1 -
 2 files changed, 15 insertions(+), 16 deletions(-)

block.c +15 ~~-15~~

index f95a59d..da1a6a9 100644
@@ -759,6 +759,21 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
     return open_flags;
 }
 
+/* This function is to find a node in the bs graph */
+static BlockDriverState *bdrv_find_node(const char *node_name)
+{
+    BlockDriverState *bs;
+
+    assert(node_name);
+
+    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
+        if (!strcmp(node_name, bs->node_name)) {
+            return bs;
+        }
+    }
+    return NULL;
+}
+
 static void bdrv_assign_node_name(BlockDriverState *bs,
                                   const char *node_name,
                                   Error **errp)

@@ -2720,21 +2735,6 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
     g_free(formats);
 }
 
-/* This function is to find a node in the bs graph */
-BlockDriverState *bdrv_find_node(const char *node_name)
-{
-    BlockDriverState *bs;
-
-    assert(node_name);
-
-    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
-        if (!strcmp(node_name, bs->node_name)) {
-            return bs;
-        }
-    }
-    return NULL;
-}
-
 /* Put this QMP function here so it can access the static graph_bdrv_states. */
 BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp)
 {

include/block/block.h +0 -1

index 1520dee..2b6e1b2 100644
@@ -404,7 +404,6 @@ int bdrv_media_changed(BlockDriverState *bs);
 void bdrv_lock_medium(BlockDriverState *bs, bool locked);
 void bdrv_eject(BlockDriverState *bs, bool eject_flag);
 const char *bdrv_get_format_name(BlockDriverState *bs);
-BlockDriverState *bdrv_find_node(const char *node_name);
 BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp);
 BlockDriverState *bdrv_lookup_bs(const char *device,
                                  const char *node_name,

Signature · 1 line

1.9.3

Jeff Cody

author

2015-10-14 13:16 UTC #10 lore raw

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

In reply to Jeff Cody

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 <[email protected]>

---
 blockdev.c        | 84 ++++++++++++++++++++-----------------------------------
 migration/block.c |  6 ++--
 2 files changed, 32 insertions(+), 58 deletions(-)

blockdev.c +30 ~~-54~~

index fe182bb..6a893e1 100644
@@ -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);

migration/block.c +2 -4

index ed865ed..6b24be8 100644
@@ -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;

Signature · 1 line

1.9.3

Jeff Cody

author

2015-10-14 13:16 UTC #1 lore raw

PATCH 1/3 block: Use bdrv_lookup_bs() instead of bdrv_find_node()

In reply to Jeff Cody

This is a precursor to making bdrv_find_node() static, and internal to block.c

There is no benefit in calling bdrv_find_node() directly. This patch replaces all calls to bdrv_find_node() outside of block.c with bdrv_lookup_bs().

Signed-off-by Jeff Cody <[email protected]>

---
 block/block-backend.c   | 2 +-
 block/mirror.c          | 2 +-
 block/write-threshold.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

block/block-backend.c +1 -1

index 2256551..7026a3f 100644
@@ -67,7 +67,7 @@ BlockBackend *blk_new(const char *name, Error **errp)
         error_setg(errp, "Device with id '%s' already exists", name);
         return NULL;
     }
-    if (bdrv_find_node(name)) {
+    if (bdrv_lookup_bs(NULL, name, NULL)) {
         error_setg(errp,
                    "Device name '%s' conflicts with an existing node name",
                    name);

block/mirror.c +1 -1

index 7e43511..cb3c765 100644
@@ -644,7 +644,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
     if (s->replaces) {
         AioContext *replace_aio_context;
 
-        s->to_replace = bdrv_find_node(s->replaces);
+        s->to_replace = bdrv_lookup_bs(NULL, s->replaces, NULL);
         if (!s->to_replace) {
             error_setg(errp, "Node name '%s' not found", s->replaces);
             return;

block/write-threshold.c +1 -1

index a53c1f5..908fa7f 100644
@@ -110,7 +110,7 @@ void qmp_block_set_write_threshold(const char *node_name,
     BlockDriverState *bs;
     AioContext *aio_context;
 
-    bs = bdrv_find_node(node_name);
+    bs = bdrv_lookup_bs(NULL, node_name, NULL);
     if (!bs) {
         error_setg(errp, "Device '%s' not found", node_name);
         return;

Signature · 1 line

1.9.3

Max Reitz

Reviewed-by on v1 1/3

2015-10-14 17:29 UTC #2 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 1/3] block: Use bdrv_lookup_bs() instead of bdrv_find_node()

Quoted 12 lines · Jeff Cody

This is a precursor to making bdrv_find_node() static, and internal to block.c
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.
There is no benefit in calling bdrv_find_node() directly. This patch replaces all calls to bdrv_find_node() outside of block.c with bdrv_lookup_bs().

Quoted 6 lines

Signed-off-by Jeff Cody <[email protected]>

---
 block/block-backend.c   | 2 +-
 block/mirror.c          | 2 +-
 block/write-threshold.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by Max Reitz <[email protected]>

Max Reitz

2015-10-14 17:35 UTC #3 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 1/3] block: Use bdrv_lookup_bs() instead of bdrv_find_node()

Quoted 13 lines · Max Reitz

Quoted 12 lines · Jeff Cody
This is a precursor to making bdrv_find_node() static, and internal to block.c
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.
There is no benefit in calling bdrv_find_node() directly. This patch replaces all calls to bdrv_find_node() outside of block.c with bdrv_lookup_bs().

Quoted 8 lines

Quoted 6 lines
Signed-off-by Jeff Cody <[email protected]>
---
 block/block-backend.c   | 2 +-
 block/mirror.c          | 2 +-
 block/write-threshold.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
Reviewed-by Max Reitz <[email protected]>

Oh, wait, on patch 2 gcc tells me I should take that back. If this series is based on Berto's series, that means it's also based on my BlockBackend series, and that one adds another instance of bdrv_find_node().

Since I'll have to send a v7 anyway, I'll make it a bdrv_lookup_bs() there, so my R-b stands.

Max

Max Reitz

Reviewed-by on v1 2/3

2015-10-14 17:41 UTC #8 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 2/3] block: make bdrv_find_node() static

In reply to Jeff Cody

Quoted 9 lines · Jeff Cody

This patch does two things: it moves bdrv_find_node() up before the first usage in block.c, and it makes the function static so that it is only internal to block.c.
Signed-off-by Jeff Cody <[email protected]>
---
 block.c               | 30 +++++++++++++++---------------
 include/block/block.h |  1 -
 2 files changed, 15 insertions(+), 16 deletions(-)

Reviewed-by Max Reitz <[email protected]>

Jeff Cody

author

2015-10-14 17:42 UTC #4 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 1/3] block: Use bdrv_lookup_bs() instead of bdrv_find_node()

In reply to Max Reitz

Quoted 23 lines · Max Reitz

Quoted 13 lines · Max Reitz
Quoted 12 lines · Jeff Cody
This is a precursor to making bdrv_find_node() static, and internal to block.c
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.
There is no benefit in calling bdrv_find_node() directly. This patch replaces all calls to bdrv_find_node() outside of block.c with bdrv_lookup_bs().
Quoted 8 lines
Quoted 6 lines
Signed-off-by Jeff Cody <[email protected]>
---
 block/block-backend.c   | 2 +-
 block/mirror.c          | 2 +-
 block/write-threshold.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
Reviewed-by Max Reitz <[email protected]>

Quoted 9 lines

Oh, wait, on patch 2 gcc tells me I should take that back. If this series is based on Berto's series, that means it's also based on my BlockBackend series, and that one adds another instance of bdrv_find_node().
Since I'll have to send a v7 anyway, I'll make it a bdrv_lookup_bs() there, so my R-b stands.
Max

Thanks. I actually managed to apply Berto's without yours, but I just went back and rectified that, and applied yours.

Jeff

Max Reitz

2015-10-14 18:05 UTC #11 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] block: use bdrv_lookup_bs() over blk_by_name() for BDS only results

In reply to Jeff Cody

Quoted 9 lines · Jeff Cody

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.

Quoted 5 lines

Signed-off-by Jeff Cody <[email protected]>

---
 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

author

2015-10-14 18:13 UTC #12 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] block: use bdrv_lookup_bs() over blk_by_name() for BDS only results

Quoted 20 lines · Max Reitz

Quoted 9 lines · Jeff Cody
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.
Quoted 5 lines
Signed-off-by Jeff Cody <[email protected]>
---
 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.

Quoted 7 lines

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.

Quoted 15 lines

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

Alberto Garcia

Reviewed-by on v1 1/3

2015-10-15 12:02 UTC #5 lore raw

Re: [Qemu-devel] [PATCH 1/3] block: Use bdrv_lookup_bs() instead of bdrv_find_node()

In reply to Jeff Cody

Quoted 8 lines · Jeff Cody

This is a precursor to making bdrv_find_node() static, and internal to block.c
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.

Quoted 5 lines

There is no benefit in calling bdrv_find_node() directly. This patch replaces all calls to bdrv_find_node() outside of block.c with bdrv_lookup_bs().
Signed-off-by Jeff Cody <[email protected]>

Reviewed-by Alberto Garcia <[email protected]>

Alberto Garcia

Reviewed-by on v1 2/3

2015-10-15 12:02 UTC #9 lore raw

Re: [Qemu-devel] [PATCH 2/3] block: make bdrv_find_node() static

In reply to Jeff Cody

Quoted 5 lines · Jeff Cody

This patch does two things: it moves bdrv_find_node() up before the first usage in block.c, and it makes the function static so that it is only internal to block.c.
Signed-off-by Jeff Cody <[email protected]>

Reviewed-by Alberto Garcia <[email protected]>

Markus Armbruster

2015-10-16 07:53 UTC #6 lore raw

Re: [Qemu-devel] [PATCH 1/3] block: Use bdrv_lookup_bs() instead of bdrv_find_node()

In reply to Jeff Cody

Quoted 24 lines · Jeff Cody

This is a precursor to making bdrv_find_node() static, and internal to block.c
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.
There is no benefit in calling bdrv_find_node() directly. This patch replaces all calls to bdrv_find_node() outside of block.c with bdrv_lookup_bs().
Signed-off-by Jeff Cody <[email protected]>
---
 block/block-backend.c   | 2 +-
 block/mirror.c          | 2 +-
 block/write-threshold.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
block/block-backend.c +0 -0
index 2256551..7026a3f 100644

block/block-backend.c BlockBackend *blk_new(const char *name, Error **errp) · line 73

Quoted 8 lines

block/block-backend.c +1 -1

@@ -67,7 +67,7 @@ BlockBackend *blk_new(const char *name, Error **errp)
         error_setg(errp, "Device with id '%s' already exists", name);
         return NULL;
     }
-    if (bdrv_find_node(name)) {
+    if (bdrv_lookup_bs(NULL, name, NULL)) {
         error_setg(errp,
                    "Device name '%s' conflicts with an existing node name",
                    name);

Here, you ignore bdrv_lookup_bs() errors because we actually succeed on bdrv_lookup_bs() error. Good.

block/mirror.c static void mirror_complete(BlockJob *job, Error **errp) · line 650

Quoted 13 lines

block/mirror.c +1 -1

index 7e43511..cb3c765 100644
@@ -644,7 +644,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
     if (s->replaces) {
         AioContext *replace_aio_context;
 
-        s->to_replace = bdrv_find_node(s->replaces);
+        s->to_replace = bdrv_lookup_bs(NULL, s->replaces, NULL);
         if (!s->to_replace) {
             error_setg(errp, "Node name '%s' not found", s->replaces);
             return;

Here, you ignore its errors because the caller sets a better one. However, the callers is better only because bdrv_lookup_bs()'s sucks:
"Cannot find device= or node_name=FOO". Follow-up patch to improve that error and use it here?

block/write-threshold.c void qmp_block_set_write_threshold(const char *node_name, · line 116

Quoted 13 lines

block/write-threshold.c +1 -1

index a53c1f5..908fa7f 100644
@@ -110,7 +110,7 @@ void qmp_block_set_write_threshold(const char *node_name,
     BlockDriverState *bs;
     AioContext *aio_context;
 
-    bs = bdrv_find_node(node_name);
+    bs = bdrv_lookup_bs(NULL, node_name, NULL);
     if (!bs) {
         error_setg(errp, "Device '%s' not found", node_name);
         return;

Likewise.