Re: [Qemu-devel] [PATCH] qapi: Add "backing" and "file" to BlockStats

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2832 bytes --]

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 <famz@redhat.com>
> ---
>  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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]