[v3,09/11] block: Don't query all block devices at hmp_nbd_server_start

Commit Message

Fabiano Rosas April 9, 2024, 2:59 p.m. UTC

We're currently doing a full query-block just to enumerate the devices
for qmp_nbd_server_add and then discarding the BlockInfoList
afterwards. Alter hmp_nbd_server_start to instead iterate explicitly
over the block_backends list.

This allows the removal of the dependency on qmp_query_block from
hmp_nbd_server_start. This is desirable because we're about to move
qmp_query_block into a coroutine and don't need to change the NBD code
at the same time.

Add the GRAPH_RDLOCK_GUARD_MAINLOOP macro because
bdrv_skip_implicit_filters() needs the graph lock.

Signed-off-by: Fabiano Rosas <[e-mail]>
---
- add a comment explaining some checks are done to preserve previous
  behavior;

- we need the strdup when assigning .device to preserve const. Just
  add a matching g_free();

- about the possible leak at qmp_nbd_server_add() unrelated to this
  patch, commit 8675cbd68b ("nbd: Utilize QAPI_CLONE for type
  conversion") mentions that the QAPI visitor will already free
  arg->name.
---
 block/monitor/block-hmp-cmds.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

Patch

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index d954bec6f1..9db587c661 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -387,10 +387,12 @@  void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
     bool writable = qdict_get_try_bool(qdict, "writable", false);
     bool all = qdict_get_try_bool(qdict, "all", false);
     Error *local_err = NULL;
-    BlockInfoList *block_list, *info;
+    BlockBackend *blk;
     SocketAddress *addr;
     NbdServerAddOptions export;
 
+    GRAPH_RDLOCK_GUARD_MAINLOOP();
+
     if (writable && !all) {
         error_setg(&local_err, "-w only valid together with -a");
         goto exit;
@@ -415,29 +417,43 @@  void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
     /* Then try adding all block devices.  If one fails, close all and
      * exit.
      */
-    block_list = qmp_query_block(NULL);
+    for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) {
+        BlockDriverState *bs = blk_bs(blk);
 
-    for (info = block_list; info; info = info->next) {
-        if (!info->value->inserted) {
+        if (!*blk_name(blk)) {
+            continue;
+        }
+
+        /*
+         * Note: historically we used to call qmp_query_block() to get
+         * the list of block devices. The two 'continue' cases below
+         * are the same as used by that function and are here to
+         * preserve behavior.
+         */
+
+        if (!blk_get_attached_dev(blk)) {
+            continue;
+        }
+
+        bs = bdrv_skip_implicit_filters(bs);
+        if (!bs || !bs->drv) {
             continue;
         }
 
         export = (NbdServerAddOptions) {
-            .device         = info->value->device,
+            .device         = g_strdup(blk_name(blk)),
             .has_writable   = true,
             .writable       = writable,
         };
 
         qmp_nbd_server_add(&export, &local_err);
-
+        g_free(export.device);
         if (local_err != NULL) {
             qmp_nbd_server_stop(NULL);
             break;
         }
     }
 
-    qapi_free_BlockInfoList(block_list);
-
 exit:
     hmp_handle_error(mon, local_err);
 }