On 09.06.23 22:19, Fabiano Rosas wrote:
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.
Signed-off-by: Fabiano Rosas
---
block/monitor/block-hmp-cmds.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index ca2599de44..26116fe831 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -394,7 +394,7 @@ 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;
@@ -419,18 +419,24 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
return;
}
-/* Then try adding all block devices. If one fails, close all and
+/*
+ * 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) && !blk_get_attached_dev(blk)) {
I’d like a comment here that historically, we’ve used qmp_query_block()
here, and this is the same condition that it uses. (Otherwise, it’s
hard to see why it matters whether a device is attached or not.)
+continue;
+}
+
+bs = bdrv_skip_implicit_filters(bs);
+if (!bs || !bs->drv) {
Same here. Just checking blk_is_inserted() would make more sense in
this place, but if you want to absolutely keep behavior unchanged, then
there should be a comment here why this check is done (because
bdrv_query_info() does it to determine whether info->inserted should be
set, which was historically used to determine whether this BlockBackend
can be exported).
continue;
}
export = (NbdServerAddOptions) {
-.device = info->value->device,
+.device = g_strdup(blk_name(blk)),
Do we need to g_strdup() here? We didn’t before, so I think this will
leak export.device.
I know bdrv_query_info() uses g_strdup(), but that was released by the
qapi_free_BlockInfoList() below, which is now removed without replacement.
(On that note, it also looks like qmp_nbd_server_add() can leak
arg->name (i.e. device.name) if it is not set by the caller. It also
uses g_strdup() there, but never frees it. It does free the export_opts
it creates, and `arg` is put into it, but as a deep copy, so anything in
`arg` is leaked.)
Hanna
.has_writable = true,
.writable = writable,
};
@@ -443,8 +449,6 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
}
}
-qapi_free_BlockInfoList(block_list);
-
exit:
hmp_handle_error(mon, local_err);
}
