[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qapi: Add "backing" and "file" to BlockStats
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH] qapi: Add "backing" and "file" to BlockStats |
Date: |
Wed, 22 Jan 2014 06:07:08 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 |
On 01/21/2014 08:30 PM, Fam Zheng wrote:
> Currently there is no way to query BlockStats of the backing chain. This
> adds "backing" and "file" fields into BlockStats to make it possible.
>
> The old optional field "parent", which has the information for "file",
> is kept there for backward compatibility, as an alias for "file".
Eww. BlockStats is a big struct (and getting bigger), which means this
is a lot of JSON duplication to send over the wire just to get discarded
(no matter whether the client uses the old or the new name).
> However field name "file" is more consistent with "file" option name in
> the block device configuration interface.
Consistency is nice, but back-compat and efficiency take higher priority
in this case. You are better off documenting that 'parent' refers to
the details of the backing file, without adding a 'file' field.
>
> Signed-off-by: Fam Zheng <address@hidden>
> ---
> block/qapi.c | 7 +++++++
> qapi-schema.json | 15 +++++++++++----
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> +++ b/block/qapi.c
> @@ -316,6 +316,13 @@ BlockStats *bdrv_query_stats(const BlockDriverState *bs)
> if (bs->file) {
> s->has_parent = true;
> s->parent = bdrv_query_stats(bs->file);
> + s->has_file = true;
> + s->file = bdrv_query_stats(bs->file);
> + }
Drop this portion.
> +
> + if (bs->backing_hd) {
> + s->has_backing = true;
> + s->backing = bdrv_query_stats(bs->backing_hd);
> }
But this one is fine.
>
> return s;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f27c48a..c3c8aec 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1022,15 +1022,22 @@
> #
> # @stats: A @BlockDeviceStats for the device.
> #
> -# @parent: #optional This may point to the backing block device if this is a
> -# a virtual block device. If it's a backing block, this will point
> -# to the backing file is one is present.
I'm okay if you improve the wording here, but keep this field untouched
with just a better description.
> +# @file: #optional This may point to the file block device if present.
> +# (Since 2.0)
Don't add this field.
> +#
> +# @backing: #optional This may point to the backing block device if present.
> +# (Since 2.0)
> +#
s/may point/describes/
> +# @parent: #optional An alias of @file for backward comatibility. (Since
> +# 0.14.0)
s/comatibility/compatibility/ - except that I don't think you want to
keep it as an alias. Also, the (Since 0.14.0) is bogus, since the field
has existed for as long as the command has.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature