[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/6] block: Add optional device argument to quer
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 2/6] block: Add optional device argument to query-block |
Date: |
Thu, 18 Sep 2014 13:53:08 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Kevin Wolf <address@hidden> writes:
> This allows querying one specific BlockDriverState instead of a list of
> all drives.
I've long resisted optional arguments to limit list-valued queries,
because I believe the additional interface complexity isn't worth the
additional utility. But resistance might be futile.
We have one only query- with an optional limiting arguement[*]:
query-rx-filter.
If we go down this route, then I'd prefer to have such optional limiting
for all query- commands returning lists of elements describing some
aspect of a named object. Preference != demand.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> block/qapi.c | 15 +++++++++++++--
> hmp.c | 6 +++---
> qapi/block-core.json | 8 ++++++--
> 3 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index 6b43bc3..2a060d3 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -367,13 +367,24 @@ static BlockStats *bdrv_query_stats(const
> BlockDriverState *bs)
> return s;
> }
>
> -BlockInfoList *qmp_query_block(Error **errp)
> +BlockInfoList *qmp_query_block(bool has_device, const char *device,
> + Error **errp)
> {
> BlockInfoList *head = NULL, **p_next = &head;
> BlockDriverState *bs = NULL;
> Error *local_err = NULL;
>
> - while ((bs = bdrv_next(bs))) {
> + if (has_device) {
> + bs = bdrv_find(device);
> + if (bs == NULL) {
> + error_setg(errp, "Device not found");
> + goto err;
> + }
> + } else {
> + bs = bdrv_next(NULL);
> + }
> +
> + for (; bs; (bs = has_device ? NULL : bdrv_next(bs))) {
> BlockInfoList *info = g_malloc0(sizeof(*info));
> bdrv_query_info(bs, &info->value, &local_err);
> if (local_err) {
I'd find
for (bs = bdrv_next(NULL); bs; bs == bdrv_next(bs)) {
if (device && strcmp(device, bs->device)) {
continue;
}
}
simpler and clearer. It's how qmp_query_rx_filter() works.
> diff --git a/hmp.c b/hmp.c
> index c3846b8..02eee80 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -298,7 +298,7 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
> const char *device = qdict_get_try_str(qdict, "device");
> bool verbose = qdict_get_try_bool(qdict, "verbose", 0);
>
> - block_list = qmp_query_block(NULL);
> + block_list = qmp_query_block(false, NULL, NULL);
>
> for (info = block_list; info; info = info->next) {
> if (device && strcmp(device, info->value->device)) {
> @@ -847,7 +847,7 @@ void hmp_cont(Monitor *mon, const QDict *qdict)
> BlockInfoList *bdev_list, *bdev;
> Error *err = NULL;
>
> - bdev_list = qmp_query_block(NULL);
> + bdev_list = qmp_query_block(false, NULL, NULL);
> for (bdev = bdev_list; bdev; bdev = bdev->next) {
> if (key_is_missing(bdev->value)) {
> monitor_read_block_device_key(mon, bdev->value->device,
> @@ -1588,7 +1588,7 @@ 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);
> + block_list = qmp_query_block(false, NULL, NULL);
>
> for (info = block_list; info; info = info->next) {
> if (!info->value->has_inserted) {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c53b587..ddc3fe0 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -354,13 +354,17 @@
> ##
> # @query-block:
> #
> -# Get a list of BlockInfo for all virtual block devices.
> +# Get a list of BlockInfo for all or one specific virtual block device.
> +#
> +# @device: #optional The id of the block device to inspect. (Since 2.2)
> #
> # Returns: a list of @BlockInfo describing each virtual block device
> #
> # Since: 0.14.0
> ##
This is how we document query-rx-filter:
##
# @query-rx-filter:
#
# Return rx-filter information for all NICs (or for the given NIC).
#
# @name: #optional net client name
#
# Returns: list of @RxFilterInfo for all NICs (or for the given NIC).
# Returns an error if the given @name doesn't exist, or given
# NIC doesn't support rx-filter querying, or given net client
# isn't a NIC.
#
# Since: 1.6
##
I find that slightly clearer. Perhaps you'd like to steal from it.
> -{ 'command': 'query-block', 'returns': ['BlockInfo'] }
> +{ 'command': 'query-block',
> + 'data': { '*device': 'str' },
> + 'returns': ['BlockInfo'] }
>
> ##
> # @BlockDeviceStats:
[*] Yes, I objected to the argument, but I didn't insist. I won't
insist now.
- [Qemu-devel] [PATCH 0/6] block: Single-device query-block(-node) and cache info, Kevin Wolf, 2014/09/16
- [Qemu-devel] [PATCH 1/6] block/qapi: Add cache information to query-block, Kevin Wolf, 2014/09/16
- [Qemu-devel] [PATCH 2/6] block: Add optional device argument to query-block, Kevin Wolf, 2014/09/16
- Re: [Qemu-devel] [PATCH 2/6] block: Add optional device argument to query-block,
Markus Armbruster <=
- [Qemu-devel] [PATCH 3/6] block: Introduce query-block-node, Kevin Wolf, 2014/09/16
- [Qemu-devel] [PATCH 4/6] block/hmp: Factor out print_block_info(), Kevin Wolf, 2014/09/16
- [Qemu-devel] [PATCH 5/6] block/hmp: Allow info = NULL in print_block_info(), Kevin Wolf, 2014/09/16
- [Qemu-devel] [PATCH 6/6] block: Allow node-name in HMP 'info block', Kevin Wolf, 2014/09/16