[Qemu-devel] [PATCH 0/2] block: add blk_lookup() for getting device by node_name

[Qemu-devel] [PATCH 0/2] block: add blk_lookup() for getting device by node_name

[Anton Kuchin]

Some HMP and QMP commands are targeting BlockBackend but
for hotplugged devices name of BB is deprecated, instead
name of root BlockDriverState is set. These patches add
functions to search BB by attached root BDS name.

This approach isn't perfect, but I couldn't invent a better
one and I belive it's more convinient than accessing BB
by QOM path.

Anton Kuchin (2):
  block: add functions to search BlockBackend by root BDS name
  block: migrate callers from blk_by_name to blk_lookup

 block/block-backend.c          | 29 +++++++++++++++++++++++++++++
 blockdev-nbd.c                 |  2 +-
 blockdev.c                     |  6 +++---
 hmp.c                          |  2 +-
 include/sysemu/block-backend.h |  7 +++++++
 5 files changed, 41 insertions(+), 5 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH 1/2] block: add functions to search BlockBackend by root BDS name

[Anton Kuchin]

BlockBackend name is empty if it is added with '-blockdev' and '-device'
options or hotplugged with QMP but callers still expect backend to be
accesible by name for operations like commit or statistics access.
Intoduce blk_lookup function to search both by name and BDS-root node_name.

Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 block/block-backend.c          | 29 +++++++++++++++++++++++++++++
 include/sysemu/block-backend.h |  7 +++++++
 2 files changed, 36 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 60d37a0c3d..86a492853c 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -684,6 +684,35 @@ static BlockBackend *bdrv_first_blk(BlockDriverState *bs)
     return NULL;
 }
 
+/*
+ * Return the BlockBackend that has attached BDS-tree root with
+ * node_name @node_name if it exists, else null.
+ * @node_name must not be null.
+ */
+static BlockBackend *blk_by_root_name(const char *node_name)
+{
+    BlockBackend *blk = NULL;
+
+    assert(node_name);
+    while ((blk = blk_all_next(blk)) != NULL) {
+        BlockDriverState *bs = blk_bs(blk);
+        if (bs && !strcmp(node_name, bs->node_name)) {
+            return blk;
+        }
+    }
+    return NULL;
+}
+
+BlockBackend *blk_lookup(const char *name)
+{
+    assert(name);
+    BlockBackend *blk = blk_by_name(name);
+    if (!blk) {
+        blk = blk_by_root_name(name);
+    }
+    return blk;
+}
+
 /*
  * Returns true if @bs has an associated BlockBackend.
  */
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c96bcdee14..290b8f8fc9 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -85,6 +85,13 @@ void blk_unref(BlockBackend *blk);
 void blk_remove_all_bs(void);
 const char *blk_name(const BlockBackend *blk);
 BlockBackend *blk_by_name(const char *name);
+
+/*
+ * Search BlockBackend by name or root BlockDriverSate node_name.
+ * Hotplug BlockBackends have no name so need to also check BDS-tree roots
+ * @name must not be null.
+ */
+BlockBackend *blk_lookup(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
 BlockBackend *blk_all_next(BlockBackend *blk);
 bool monitor_add_blk(BlockBackend *blk, const char *name, Error **errp);
-- 
2.19.1

[Qemu-devel] [PATCH 2/2] block: migrate callers from blk_by_name to blk_lookup

[Anton Kuchin]

Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 blockdev-nbd.c | 2 +-
 blockdev.c     | 6 +++---
 hmp.c          | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index d73ac1b026..f2ea7318cf 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -162,7 +162,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
         return;
     }
 
-    on_eject_blk = blk_by_name(device);
+    on_eject_blk = blk_lookup(device);
 
     bs = bdrv_lookup_bs(device, device, errp);
     if (!bs) {
diff --git a/blockdev.c b/blockdev.c
index a8fa8748a9..bb01c41038 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1082,7 +1082,7 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
         BlockDriverState *bs;
         AioContext *aio_context;
 
-        blk = blk_by_name(device);
+        blk = blk_lookup(device);
         if (!blk) {
             monitor_printf(mon, "Device '%s' not found\n", device);
             return;
@@ -3066,7 +3066,7 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    blk = blk_by_name(id);
+    blk = blk_lookup(id);
     if (!blk) {
         error_report("Device '%s' not found", id);
         return;
@@ -4431,7 +4431,7 @@ void qmp_x_block_latency_histogram_set(
     bool has_boundaries_flush, uint64List *boundaries_flush,
     Error **errp)
 {
-    BlockBackend *blk = blk_by_name(device);
+    BlockBackend *blk = blk_lookup(device);
     BlockAcctStats *stats;
     int ret;
 
diff --git a/hmp.c b/hmp.c
index b2a2b1f84e..7b03d5c1d7 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2460,7 +2460,7 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
     int ret;
 
-    blk = blk_by_name(device);
+    blk = blk_lookup(device);
     if (!blk) {
         BlockDriverState *bs = bdrv_lookup_bs(NULL, device, &err);
         if (bs) {
-- 
2.19.1

Re: [Qemu-devel] [PATCH 0/2] block: add blk_lookup() for getting device by node_name

[Kevin Wolf]

Am 28.01.2019 um 15:27 hat Anton Kuchin geschrieben:
> Some HMP and QMP commands are targeting BlockBackend but
> for hotplugged devices name of BB is deprecated, instead
> name of root BlockDriverState is set. These patches add
> functions to search BB by attached root BDS name.
> 
> This approach isn't perfect, but I couldn't invent a better
> one and I belive it's more convinient than accessing BB
> by QOM path.

There could be more than one BlockBackend attached to a single node, so
this approach is simply wrong.

I think the qdev ID is actually not only a pretty convenient way, but in
fact the only logical way to address a guest device (and BlockBackends
that can be accessed by the monitor are essentially a part of the guest
device).

Does your series allow to perform any operation that isn't possible
currently? If so, it's probably this operation that needs to be fixed to
either accept node names (if it's a backend operation) or a device ID
(if it's a frontend operation).

Kevin

Re: [Qemu-devel] [PATCH 0/2] block: add blk_lookup() for getting device by node_name

[Anton Kuchin]

On 28/01/2019 17:47, Kevin Wolf wrote:
> Am 28.01.2019 um 15:27 hat Anton Kuchin geschrieben:
>> Some HMP and QMP commands are targeting BlockBackend but
>> for hotplugged devices name of BB is deprecated, instead
>> name of root BlockDriverState is set. These patches add
>> functions to search BB by attached root BDS name.
>>
>> This approach isn't perfect, but I couldn't invent a better
>> one and I belive it's more convinient than accessing BB
>> by QOM path.
> There could be more than one BlockBackend attached to a single node, so
> this approach is simply wrong.

I was thinking about this but couldn't imagine configuration when it's 
having more than one root. Can you give an example, please, so I 
understand this case better.

> I think the qdev ID is actually not only a pretty convenient way, but in
> fact the only logical way to address a guest device (and BlockBackends
> that can be accessed by the monitor are essentially a part of the guest
> device).

As far as I remember backends currently have emply qdev ID so the only 
way to address them now is QOM path like 
".../my_hotplug_drive/virtio-backend". So I have to remember the name of 
the root driver it is connected to and also add this "/virtio-backend" 
suffix.

Can you explain a bit what are "monitor referenced" backends and which 
one can be accessed from monitor and which can not.

> Does your series allow to perform any operation that isn't possible
> currently? If so, it's probably this operation that needs to be fixed to
> either accept node names (if it's a backend operation) or a device ID
> (if it's a frontend operation).

Yes. It fixes latency histogram setting, nbd server binding to remove 
event and a couple of hmp comands. I also suspect that this can affect 
migration but I could't figure out what name is used for identifying drives.

Some of this code is also linked to utilities (qemu-img, qemu-io-cmds 
...) which require linking all QAPI stuff to them, that seems to be an 
overkill. I'll prepare the patch to eliminate this dependency and allow 
using QOM here if we decide that this is the best way.

> Kevin
>

Re: [Qemu-devel] [PATCH 0/2] block: add blk_lookup() for getting device by node_name

[Kevin Wolf]

Am 28.01.2019 um 17:53 hat Anton Kuchin geschrieben:
> On 28/01/2019 17:47, Kevin Wolf wrote:
> > Am 28.01.2019 um 15:27 hat Anton Kuchin geschrieben:
> > > Some HMP and QMP commands are targeting BlockBackend but
> > > for hotplugged devices name of BB is deprecated, instead
> > > name of root BlockDriverState is set. These patches add
> > > functions to search BB by attached root BDS name.
> > > 
> > > This approach isn't perfect, but I couldn't invent a better
> > > one and I belive it's more convinient than accessing BB
> > > by QOM path.
> > There could be more than one BlockBackend attached to a single node, so
> > this approach is simply wrong.
> 
> I was thinking about this but couldn't imagine configuration when it's
> having more than one root. Can you give an example, please, so I understand
> this case better.

You can configure such setups explicitly by just using the same -device
drive=node-name twice, though I admit this is unusual.

The more practical case are internal BlockBackends for use by block jobs
or the NBD server. In that case, the user might modify a completely
different BlockBackend than they had in mind.

> > I think the qdev ID is actually not only a pretty convenient way, but in
> > fact the only logical way to address a guest device (and BlockBackends
> > that can be accessed by the monitor are essentially a part of the guest
> > device).
> 
> As far as I remember backends currently have emply qdev ID so the only way
> to address them now is QOM path like ".../my_hotplug_drive/virtio-backend".
> So I have to remember the name of the root driver it is connected to and
> also add this "/virtio-backend" suffix.

virtio-blk is ugly, indeed, because the device is split in two halves
and the half with the assigned ID isn't the half that owns the
BlockBackend. But the paths are relative to /machine/peripheral, so I
think just "my_hotplug_drive/virtio-backend" should work.

For all other devices, it's just the ID, as far as I know.

> Can you explain a bit what are "monitor referenced" backends and which one
> can be accessed from monitor and which can not.

The user isn't supposed to access internal BlockBackends, like those of
block jobs and the NBD server.

> > Does your series allow to perform any operation that isn't possible
> > currently? If so, it's probably this operation that needs to be fixed to
> > either accept node names (if it's a backend operation) or a device ID
> > (if it's a frontend operation).
> 
> Yes. It fixes latency histogram setting, nbd server binding to remove event
> and a couple of hmp comands. I also suspect that this can affect migration
> but I could't figure out what name is used for identifying drives.

qmp_x_block_latency_histogram_set() uses blk_by_name() to get the
BlockBackend. The fix is to make it use qmp_get_blk() instead.

I'm not sure what the exact NBD thing is you mean.

Kevin