Re: [PATCH v2 08/10] block: Don't query all block devices at hmp_nbd_server_start

From: Hanna Czenczek <hreitz@redhat.com>
To: Fabiano Rosas <farosas@suse.de>, qemu-devel@nongnu.org
Cc: qemu-block@nongnu.org, "Kevin Wolf" <kwolf@redhat.com>,
	"Markus Armbruster" <armbru@redhat.com>,
	"João Silva" <jsilva@suse.de>, "Lin Ma" <lma@suse.com>,
	"Claudio Fontana" <cfontana@suse.de>,
	"Dario Faggioli" <dfaggioli@suse.com>,
	"Eric Blake" <eblake@redhat.com>
Subject: Re: [PATCH v2 08/10] block: Don't query all block devices at hmp_nbd_server_start
Date: Mon, 6 Nov 2023 14:07:29 +0100	[thread overview]
Message-ID: <8c00b1f8-dd80-481f-9474-9514c40cd5fe@redhat.com> (raw)
In-Reply-To: <20230609201910.12100-9-farosas@suse.de>

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 <farosas@suse.de>
> ---
>   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);
>   }