| Message ID | 20230609201910.12100-9-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Make raw_co_get_allocated_file_size asynchronous | expand |
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); > }
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)) { + 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, }; @@ -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); }
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(-)