[Qemu-devel] [PATCH 0/3] Prefer bdrv_lookup_bs() to find BDS nodes

[Qemu-devel] [PATCH 0/3] Prefer bdrv_lookup_bs() to find BDS nodes

[Jeff Cody]

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

-- 
1.9.3

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

[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 <jcody@redhat.com>
---
 block/block-backend.c   | 2 +-
 block/mirror.c          | 2 +-
 block/write-threshold.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 2256551..7026a3f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -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);
diff --git a/block/mirror.c b/block/mirror.c
index 7e43511..cb3c765 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -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;
diff --git a/block/write-threshold.c b/block/write-threshold.c
index a53c1f5..908fa7f 100644
--- a/block/write-threshold.c
+++ b/block/write-threshold.c
@@ -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;
-- 
1.9.3

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

[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 <jcody@redhat.com>
---
 block.c               | 30 +++++++++++++++---------------
 include/block/block.h |  1 -
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index f95a59d..da1a6a9 100644
--- a/block.c
+++ b/block.c
@@ -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)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 1520dee..2b6e1b2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -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,
-- 
1.9.3

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

[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 <jcody@redhat.com>
---
 blockdev.c        | 84 ++++++++++++++++++++-----------------------------------
 migration/block.c |  6 ++--
 2 files changed, 32 insertions(+), 58 deletions(-)

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

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

On 14.10.2015 15:16, Jeff Cody wrote:
> 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 <jcody@redhat.com>
> ---
>  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 <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

On 14.10.2015 19:29, Max Reitz wrote:
> On 14.10.2015 15:16, Jeff Cody wrote:
>> 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 <jcody@redhat.com>
>> ---
>>  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 <mreitz@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 469 bytes --]

On 14.10.2015 15:16, Jeff Cody wrote:
> 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 <jcody@redhat.com>
> ---
>  block.c               | 30 +++++++++++++++---------------
>  include/block/block.h |  1 -
>  2 files changed, 15 insertions(+), 16 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Jeff Cody]

On Wed, Oct 14, 2015 at 07:34:43PM +0200, Max Reitz wrote:
> On 14.10.2015 19:29, Max Reitz wrote:
> > On 14.10.2015 15:16, Jeff Cody wrote:
> >> 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 <jcody@redhat.com>
> >> ---
> >>  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 <mreitz@redhat.com>
> 
> 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

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2057 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Jeff Cody]

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
> 

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

[Alberto Garcia]

On Wed 14 Oct 2015 03:16:00 PM CEST, Jeff Cody <jcody@redhat.com> wrote:
> 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 <jcody@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

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

[Alberto Garcia]

On Wed 14 Oct 2015 03:16:01 PM CEST, Jeff Cody wrote:
> 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 <jcody@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

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

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> 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 <jcody@redhat.com>
> ---
>  block/block-backend.c   | 2 +-
>  block/mirror.c          | 2 +-
>  block/write-threshold.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 2256551..7026a3f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -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.

> diff --git a/block/mirror.c b/block/mirror.c
> index 7e43511..cb3c765 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -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?

> diff --git a/block/write-threshold.c b/block/write-threshold.c
> index a53c1f5..908fa7f 100644
> --- a/block/write-threshold.c
> +++ b/block/write-threshold.c
> @@ -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.