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

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

[Fam Zheng]

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".
However field name "file" is more consistent with "file" option name in
the block device configuration interface.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/qapi.c     |  7 +++++++
 qapi-schema.json | 15 +++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index a32cb79..858650e 100644
--- a/block/qapi.c
+++ 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);
+    }
+
+    if (bs->backing_hd) {
+        s->has_backing = true;
+        s->backing = bdrv_query_stats(bs->backing_hd);
     }
 
     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.
+# @file:   #optional This may point to the file block device if present.
+#          (Since 2.0)
+#
+# @backing: #optional This may point to the backing block device if present.
+#           (Since 2.0)
+#
+# @parent: #optional An alias of @file for backward comatibility. (Since
+#          0.14.0)
 #
 # Since: 0.14.0
 ##
 { 'type': 'BlockStats',
   'data': {'*device': 'str', 'stats': 'BlockDeviceStats',
-           '*parent': 'BlockStats'} }
+           '*parent': 'BlockStats',
+           '*file': 'BlockStats',
+           '*backing': 'BlockStats'} }
 
 ##
 # @query-blockstats:
-- 
1.8.5.3

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

[Kevin Wolf]

Am 22.01.2014 um 04:30 hat Fam Zheng geschrieben:
> 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".
> However field name "file" is more consistent with "file" option name in
> the block device configuration interface.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

I don't think it's a good idea to duplicate a field just because we like
the new name better. Eric?

Kevin

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

[Eric Blake]

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

On 01/22/2014 02:35 AM, Kevin Wolf wrote:
> Am 22.01.2014 um 04:30 hat Fam Zheng geschrieben:
>> 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".
>> However field name "file" is more consistent with "file" option name in
>> the block device configuration interface.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> I don't think it's a good idea to duplicate a field just because we like
> the new name better. Eric?

I concur - duplication for the sake of naming consistency doesn't add
any measurable benefit; I'd rather just improve the docs for the
existing name (libvirt will be forced to use the old name for
back-compat reasons anyway, making the new name useless cruft that just
makes the JSON longer and cost more cycles to parse then discard).

-- 
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 --]

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

[Eric Blake]

[-- 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 --]