Patchwork [V2,2/3] block: Use bdrv_get_backing_file_ancestors_count()

login
register
mail settings
Submitter Benoit Canet
Date July 25, 2012, 8:11 a.m.
Message ID <1343203902-10524-3-git-send-email-benoit@irqsave.net>
Download mbox | patch
Permalink /patch/173120/
State New
Headers show

Comments

Benoit Canet - July 25, 2012, 8:11 a.m.
From: Benoît Canet <benoit@irqsave.net>

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

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block.c          |    2 ++
 qapi-schema.json |    9 ++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)
Kevin Wolf - July 25, 2012, 10:57 a.m.
Am 25.07.2012 10:11, schrieb benoit.canet@gmail.com:
> From: Benoît Canet <benoit@irqsave.net>
> 
> Use the dedicated counting function in qmp_query_block in order to
> propagate the backing file count to HMP.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c          |    2 ++
>  qapi-schema.json |    9 ++++++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 03e0860..4aa3ea9 100644
> --- a/block.c
> +++ b/block.c
> @@ -2448,6 +2448,8 @@ BlockInfoList *qmp_query_block(Error **errp)
>              if (bs->backing_file[0]) {
>                  info->value->inserted->has_backing_file = true;
>                  info->value->inserted->backing_file = g_strdup(bs->backing_file);
> +                info->value->inserted->backing_file_ancestors_count =
> +                    bdrv_get_backing_file_ancestors_count(bs);
>              }
>  
>              if (bs->io_limits_enabled) {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a92adb1..eb72c16 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_ancestors_count: #optional the count of ancestors backing files (for copy-on-write)
> +#

Why is it optional? Would it be omitted rather than set to 0 if there
are no backing files?

Could also use a "(since: 1.2)" note.

Kevin
Benoît Canet - July 25, 2012, 11:15 a.m.
Le Wednesday 25 Jul 2012 à 12:57:05 (+0200), Kevin Wolf a écrit :
> Am 25.07.2012 10:11, schrieb benoit.canet@gmail.com:
> > From: Benoît Canet <benoit@irqsave.net>
> > 
> > Use the dedicated counting function in qmp_query_block in order to
> > propagate the backing file count to HMP.
> > 
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  block.c          |    2 ++
> >  qapi-schema.json |    9 ++++++---
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 03e0860..4aa3ea9 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2448,6 +2448,8 @@ BlockInfoList *qmp_query_block(Error **errp)
> >              if (bs->backing_file[0]) {
> >                  info->value->inserted->has_backing_file = true;
> >                  info->value->inserted->backing_file = g_strdup(bs->backing_file);
> > +                info->value->inserted->backing_file_ancestors_count =
> > +                    bdrv_get_backing_file_ancestors_count(bs);
> >              }
> >  
> >              if (bs->io_limits_enabled) {
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index a92adb1..eb72c16 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_ancestors_count: #optional the count of ancestors backing files (for copy-on-write)
> > +#
> 
> Why is it optional? Would it be omitted rather than set to 0 if there
> are no backing files?

I made it optional because backing_file=something is optional.
So It seemed coherent to make it also optional.
However I'll change it if you confirm it should be changed.

> 
> Could also use a "(since: 1.2)" note.
> 
> Kevin
Kevin Wolf - July 25, 2012, 11:19 a.m.
Am 25.07.2012 13:15, schrieb Benoît Canet:
> Le Wednesday 25 Jul 2012 à 12:57:05 (+0200), Kevin Wolf a écrit :
>> Am 25.07.2012 10:11, schrieb benoit.canet@gmail.com:
>>> From: Benoît Canet <benoit@irqsave.net>
>>>
>>> Use the dedicated counting function in qmp_query_block in order to
>>> propagate the backing file count to HMP.
>>>
>>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
>>> ---
>>>  block.c          |    2 ++
>>>  qapi-schema.json |    9 ++++++---
>>>  2 files changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 03e0860..4aa3ea9 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -2448,6 +2448,8 @@ BlockInfoList *qmp_query_block(Error **errp)
>>>              if (bs->backing_file[0]) {
>>>                  info->value->inserted->has_backing_file = true;
>>>                  info->value->inserted->backing_file = g_strdup(bs->backing_file);
>>> +                info->value->inserted->backing_file_ancestors_count =
>>> +                    bdrv_get_backing_file_ancestors_count(bs);
>>>              }
>>>  
>>>              if (bs->io_limits_enabled) {
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index a92adb1..eb72c16 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_ancestors_count: #optional the count of ancestors backing files (for copy-on-write)
>>> +#
>>
>> Why is it optional? Would it be omitted rather than set to 0 if there
>> are no backing files?
> 
> I made it optional because backing_file=something is optional.
> So It seemed coherent to make it also optional.
> However I'll change it if you confirm it should be changed.

The reason why the backing file naem is optional is because there is no
valid value to use when there is no backing file. But for the count, 0
makes perfect sense.

Kevin
Benoît Canet - July 25, 2012, 11:42 a.m.
Le Wednesday 25 Jul 2012 à 13:19:15 (+0200), Kevin Wolf a écrit :
> Am 25.07.2012 13:15, schrieb Benoît Canet:
> > Le Wednesday 25 Jul 2012 à 12:57:05 (+0200), Kevin Wolf a écrit :
> >> Am 25.07.2012 10:11, schrieb benoit.canet@gmail.com:
> >>> From: Benoît Canet <benoit@irqsave.net>
> >>>
> >>> Use the dedicated counting function in qmp_query_block in order to
> >>> propagate the backing file count to HMP.
> >>>
> >>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> >>> ---
> >>>  block.c          |    2 ++
> >>>  qapi-schema.json |    9 ++++++---
> >>>  2 files changed, 8 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/block.c b/block.c
> >>> index 03e0860..4aa3ea9 100644
> >>> --- a/block.c
> >>> +++ b/block.c
> >>> @@ -2448,6 +2448,8 @@ BlockInfoList *qmp_query_block(Error **errp)
> >>>              if (bs->backing_file[0]) {
> >>>                  info->value->inserted->has_backing_file = true;
> >>>                  info->value->inserted->backing_file = g_strdup(bs->backing_file);
> >>> +                info->value->inserted->backing_file_ancestors_count =
> >>> +                    bdrv_get_backing_file_ancestors_count(bs);
> >>>              }
> >>>  
> >>>              if (bs->io_limits_enabled) {
> >>> diff --git a/qapi-schema.json b/qapi-schema.json
> >>> index a92adb1..eb72c16 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_ancestors_count: #optional the count of ancestors backing files (for copy-on-write)
> >>> +#
> >>
> >> Why is it optional? Would it be omitted rather than set to 0 if there
> >> are no backing files?
> > 
> > I made it optional because backing_file=something is optional.
> > So It seemed coherent to make it also optional.
> > However I'll change it if you confirm it should be changed.
> 
> The reason why the backing file naem is optional is because there is no
> valid value to use when there is no backing file. But for the count, 0
> makes perfect sense.

Should I also make is alway present in the HMP output ?

Benoît

> 
> Kevin
>
Kevin Wolf - July 25, 2012, 12:03 p.m.
Am 25.07.2012 13:42, schrieb Benoît Canet:
> Le Wednesday 25 Jul 2012 à 13:19:15 (+0200), Kevin Wolf a écrit :
>> Am 25.07.2012 13:15, schrieb Benoît Canet:
>>> Le Wednesday 25 Jul 2012 à 12:57:05 (+0200), Kevin Wolf a écrit :
>>>> Am 25.07.2012 10:11, schrieb benoit.canet@gmail.com:
>>>>> From: Benoît Canet <benoit@irqsave.net>
>>>>>
>>>>> Use the dedicated counting function in qmp_query_block in order to
>>>>> propagate the backing file count to HMP.
>>>>>
>>>>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
>>>>> ---
>>>>>  block.c          |    2 ++
>>>>>  qapi-schema.json |    9 ++++++---
>>>>>  2 files changed, 8 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/block.c b/block.c
>>>>> index 03e0860..4aa3ea9 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -2448,6 +2448,8 @@ BlockInfoList *qmp_query_block(Error **errp)
>>>>>              if (bs->backing_file[0]) {
>>>>>                  info->value->inserted->has_backing_file = true;
>>>>>                  info->value->inserted->backing_file = g_strdup(bs->backing_file);
>>>>> +                info->value->inserted->backing_file_ancestors_count =
>>>>> +                    bdrv_get_backing_file_ancestors_count(bs);
>>>>>              }
>>>>>  
>>>>>              if (bs->io_limits_enabled) {
>>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>>> index a92adb1..eb72c16 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_ancestors_count: #optional the count of ancestors backing files (for copy-on-write)
>>>>> +#
>>>>
>>>> Why is it optional? Would it be omitted rather than set to 0 if there
>>>> are no backing files?
>>>
>>> I made it optional because backing_file=something is optional.
>>> So It seemed coherent to make it also optional.
>>> However I'll change it if you confirm it should be changed.
>>
>> The reason why the backing file naem is optional is because there is no
>> valid value to use when there is no backing file. But for the count, 0
>> makes perfect sense.
> 
> Should I also make is alway present in the HMP output ?

I think I would hide it in HMP if it is 0, but this isn't a very strong
opinion. You can do it as you like.

Kevin

Patch

diff --git a/block.c b/block.c
index 03e0860..4aa3ea9 100644
--- a/block.c
+++ b/block.c
@@ -2448,6 +2448,8 @@  BlockInfoList *qmp_query_block(Error **errp)
             if (bs->backing_file[0]) {
                 info->value->inserted->has_backing_file = true;
                 info->value->inserted->backing_file = g_strdup(bs->backing_file);
+                info->value->inserted->backing_file_ancestors_count =
+                    bdrv_get_backing_file_ancestors_count(bs);
             }
 
             if (bs->io_limits_enabled) {
diff --git a/qapi-schema.json b/qapi-schema.json
index a92adb1..eb72c16 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_ancestors_count: #optional the count of ancestors backing files (for copy-on-write)
+#
 # @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_ancestors_count': 'int',
+            'encrypted': 'bool', 'bps': 'int', 'bps_rd': 'int',
+            'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int',
+            'iops_wr': 'int'} }
 
 ##
 # @BlockDeviceIoStatus: