Patchwork [V5,2/4] block: Use bdrv_get_backing_file_depth()

login
register
mail settings
Submitter Benoit Canet
Date July 26, 2012, 1:18 p.m.
Message ID <1343308701-29632-3-git-send-email-benoit@irqsave.net>
Download mbox | patch
Permalink /patch/173438/
State New
Headers show

Comments

Benoit Canet - July 26, 2012, 1:18 p.m.
From: Benoît Canet <benoit@irqsave.net>

Use the dedicated counting function in qmp_query_block in order to
propagate the backing file depth to HMP.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block.c          |    3 +++
 qapi-schema.json |    9 ++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)
Luiz Capitulino - July 30, 2012, 4:32 p.m.
On Thu, 26 Jul 2012 15:18:19 +0200
benoit.canet@gmail.com wrote:

> From: Benoît Canet <benoit@irqsave.net>
> 
> Use the dedicated counting function in qmp_query_block in order to
> propagate the backing file depth to HMP.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c          |    3 +++
>  qapi-schema.json |    9 ++++++---
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 22ebe49..a6fba1d 100644
> --- a/block.c
> +++ b/block.c
> @@ -2450,6 +2450,9 @@ BlockInfoList *qmp_query_block(Error **errp)
>                  info->value->inserted->backing_file = g_strdup(bs->backing_file);
>              }
>  
> +            info->value->inserted->backing_file_depth =
> +                bdrv_get_backing_file_depth(bs);
> +
>              if (bs->io_limits_enabled) {
>                  info->value->inserted->bps =
>                                 bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL];
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a92adb1..2b50f22 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -398,6 +398,8 @@
>  #
>  # @backing_file: #optional the name of the backing file (for copy-on-write)
>  #
> +# @backing_file_depth: number of files in the backing file chain (since: 1.2)
> +#
>  # @encrypted: true if the backing device is encrypted
>  #
>  # @bps: total throughput limit in bytes per second is specified
> @@ -418,9 +420,10 @@
>  ##
>  { 'type': 'BlockDeviceInfo',
>    'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
> -            '*backing_file': 'str', 'encrypted': 'bool',
> -            'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
> -            'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int'} }
> +            '*backing_file': 'str', 'backing-file-depth': 'int',

Should use underscores, ie. should be backing_file_depth.

Also, the next patch and this can be squashed together.

Otherwise series looks good to me now.


> +            'encrypted': 'bool', 'bps': 'int', 'bps_rd': 'int',
> +            'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int',
> +            'iops_wr': 'int'} }
>  
>  ##
>  # @BlockDeviceIoStatus:
Eric Blake - July 30, 2012, 4:48 p.m.
On 07/30/2012 10:32 AM, Luiz Capitulino wrote:
> On Thu, 26 Jul 2012 15:18:19 +0200
> benoit.canet@gmail.com wrote:
> 
>> From: Benoît Canet <benoit@irqsave.net>
>>
>> Use the dedicated counting function in qmp_query_block in order to
>> propagate the backing file depth to HMP.
>>
>> Signed-off-by: Benoit Canet <benoit@irqsave.net>

>> +++ b/qapi-schema.json
>> @@ -398,6 +398,8 @@
>>  #
>>  # @backing_file: #optional the name of the backing file (for copy-on-write)
>>  #
>> +# @backing_file_depth: number of files in the backing file chain (since: 1.2)
>> +#
>>  # @encrypted: true if the backing device is encrypted
>>  #
>>  # @bps: total throughput limit in bytes per second is specified
>> @@ -418,9 +420,10 @@
>>  ##
>>  { 'type': 'BlockDeviceInfo',
>>    'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
>> -            '*backing_file': 'str', 'encrypted': 'bool',
>> -            'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
>> -            'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int'} }
>> +            '*backing_file': 'str', 'backing-file-depth': 'int',
> 
> Should use underscores, ie. should be backing_file_depth.

Really?  I thought we _want_ new interfaces to use '-', not '_', in QMP.
 For 'backing_file', we are stuck due to back-compat (unless Anthony's
proposed patch to parse names case-insensitively with '-' and '_' folded
together is taken), but for the new field, we have no back-compat
constraints, and I would prefer backing-file-depth.

At any rate, you definitely need to make sure you agree between the docs
above and the JSON statement below (as written, you have both spellings
in the same patch).
Luiz Capitulino - July 30, 2012, 5 p.m.
On Mon, 30 Jul 2012 10:48:49 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 07/30/2012 10:32 AM, Luiz Capitulino wrote:
> > On Thu, 26 Jul 2012 15:18:19 +0200
> > benoit.canet@gmail.com wrote:
> > 
> >> From: Benoît Canet <benoit@irqsave.net>
> >>
> >> Use the dedicated counting function in qmp_query_block in order to
> >> propagate the backing file depth to HMP.
> >>
> >> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> 
> >> +++ b/qapi-schema.json
> >> @@ -398,6 +398,8 @@
> >>  #
> >>  # @backing_file: #optional the name of the backing file (for copy-on-write)
> >>  #
> >> +# @backing_file_depth: number of files in the backing file chain (since: 1.2)
> >> +#
> >>  # @encrypted: true if the backing device is encrypted
> >>  #
> >>  # @bps: total throughput limit in bytes per second is specified
> >> @@ -418,9 +420,10 @@
> >>  ##
> >>  { 'type': 'BlockDeviceInfo',
> >>    'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
> >> -            '*backing_file': 'str', 'encrypted': 'bool',
> >> -            'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
> >> -            'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int'} }
> >> +            '*backing_file': 'str', 'backing-file-depth': 'int',
> > 
> > Should use underscores, ie. should be backing_file_depth.
> 
> Really?  I thought we _want_ new interfaces to use '-', not '_', in QMP.

The problem is mixing them. BlockDeviceInfo already uses "_", and it's
not only for backing_file.

For new commands or existing commands that don't use "_", we certainly should
use "-", but mixing them for the same command is probably a bad thing.

>  For 'backing_file', we are stuck due to back-compat (unless Anthony's
> proposed patch to parse names case-insensitively with '-' and '_' folded
> together is taken),

We can't take Anthony's patch for what we emit. We could use it for input,
but I don't think it's worth it.

> but for the new field, we have no back-compat
> constraints, and I would prefer backing-file-depth.

It's like not I can't be convinced, but I think we shouldn't mix "-" and
"_" for the same command.

> 
> At any rate, you definitely need to make sure you agree between the docs
> above and the JSON statement below (as written, you have both spellings
> in the same patch).
>

Patch

diff --git a/block.c b/block.c
index 22ebe49..a6fba1d 100644
--- a/block.c
+++ b/block.c
@@ -2450,6 +2450,9 @@  BlockInfoList *qmp_query_block(Error **errp)
                 info->value->inserted->backing_file = g_strdup(bs->backing_file);
             }
 
+            info->value->inserted->backing_file_depth =
+                bdrv_get_backing_file_depth(bs);
+
             if (bs->io_limits_enabled) {
                 info->value->inserted->bps =
                                bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL];
diff --git a/qapi-schema.json b/qapi-schema.json
index a92adb1..2b50f22 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -398,6 +398,8 @@ 
 #
 # @backing_file: #optional the name of the backing file (for copy-on-write)
 #
+# @backing_file_depth: number of files in the backing file chain (since: 1.2)
+#
 # @encrypted: true if the backing device is encrypted
 #
 # @bps: total throughput limit in bytes per second is specified
@@ -418,9 +420,10 @@ 
 ##
 { 'type': 'BlockDeviceInfo',
   'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
-            '*backing_file': 'str', 'encrypted': 'bool',
-            'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
-            'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int'} }
+            '*backing_file': 'str', 'backing-file-depth': 'int',
+            'encrypted': 'bool', 'bps': 'int', 'bps_rd': 'int',
+            'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int',
+            'iops_wr': 'int'} }
 
 ##
 # @BlockDeviceIoStatus: