diff mbox series

[RFC] qapi: Allow getting flat output from 'query-named-block-nodes'

Message ID 42dae98e1f6a9f444f48a20192f45195337824f0.1576246045.git.pkrempa@redhat.com (mailing list archive)
State New, archived
Headers show
Series [RFC] qapi: Allow getting flat output from 'query-named-block-nodes' | expand

Commit Message

Peter Krempa Dec. 13, 2019, 2:11 p.m. UTC
When a management application manages node names there's no reason to
recurse into backing images in the output of query-named-block-nodes.

Add a parameter to the command which will return just the top level
structs.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 block.c               |  5 +++--
 block/qapi.c          | 10 ++++++++--
 blockdev.c            | 12 ++++++++++--
 include/block/block.h |  2 +-
 include/block/qapi.h  |  4 +++-
 monitor/hmp-cmds.c    |  2 +-
 qapi/block-core.json  |  6 +++++-
 7 files changed, 31 insertions(+), 10 deletions(-)

Comments

Eric Blake Dec. 13, 2019, 3:23 p.m. UTC | #1
On 12/13/19 8:11 AM, Peter Krempa wrote:
> When a management application manages node names there's no reason to
> recurse into backing images in the output of query-named-block-nodes.
> 
> Add a parameter to the command which will return just the top level
> structs.

At one point, Kevin was working on a saner command that tried to cut out 
on more than just the redundant nesting.  But this is certainly a 
quick-and-easy fix to ease libvirt's use of the existing command, while 
we decide whether to add a saner new command.

> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>   block.c               |  5 +++--
>   block/qapi.c          | 10 ++++++++--
>   blockdev.c            | 12 ++++++++++--
>   include/block/block.h |  2 +-
>   include/block/qapi.h  |  4 +++-
>   monitor/hmp-cmds.c    |  2 +-
>   qapi/block-core.json  |  6 +++++-
>   7 files changed, 31 insertions(+), 10 deletions(-)
> 

> +++ b/blockdev.c
> @@ -3707,9 +3707,17 @@ void qmp_drive_backup(DriveBackup *arg, Error **errp)
>       }
>   }
> 
> -BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
> +BlockDeviceInfoList *qmp_query_named_block_nodes(bool has_flat,
> +                                                 bool flat,
> +                                                 Error **errp)
>   {
> -    return bdrv_named_nodes_list(errp);
> +    bool return_flat = false;
> +
> +    if (has_flat) {
> +        return_flat = flat;
> +    }

This could be shortened as 'bool return_flat = has_flat && flat;', but 
that's not essential.

> +
> +    return bdrv_named_nodes_list(return_flat, errp);
>   }
> 

Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster Dec. 17, 2019, 7:36 a.m. UTC | #2
Eric Blake <eblake@redhat.com> writes:

> On 12/13/19 8:11 AM, Peter Krempa wrote:
>> When a management application manages node names there's no reason to
>> recurse into backing images in the output of query-named-block-nodes.
>>
>> Add a parameter to the command which will return just the top level
>> structs.
>
> At one point, Kevin was working on a saner command that tried to cut
> out on more than just the redundant nesting.  But this is certainly a
> quick-and-easy fix to ease libvirt's use of the existing command,
> while we decide whether to add a saner new command.

What exactly is the problem libvirt is having with the existing
query-named-block-nodes?

>>
>> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>> ---
>>   block.c               |  5 +++--
>>   block/qapi.c          | 10 ++++++++--
>>   blockdev.c            | 12 ++++++++++--
>>   include/block/block.h |  2 +-
>>   include/block/qapi.h  |  4 +++-
>>   monitor/hmp-cmds.c    |  2 +-
>>   qapi/block-core.json  |  6 +++++-
>>   7 files changed, 31 insertions(+), 10 deletions(-)
>>
>
>> +++ b/blockdev.c
>> @@ -3707,9 +3707,17 @@ void qmp_drive_backup(DriveBackup *arg, Error **errp)
>>       }
>>   }
>>
>> -BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
>> +BlockDeviceInfoList *qmp_query_named_block_nodes(bool has_flat,
>> +                                                 bool flat,
>> +                                                 Error **errp)
>>   {
>> -    return bdrv_named_nodes_list(errp);
>> +    bool return_flat = false;
>> +
>> +    if (has_flat) {
>> +        return_flat = flat;
>> +    }
>
> This could be shortened as 'bool return_flat = has_flat && flat;', but
> that's not essential.

I'd prefer that.

Even return_flat = flat would do, because !has_flat implies !flat when
flat is bool.  Matter of taste.

>> +
>> +    return bdrv_named_nodes_list(return_flat, errp);
>>   }
>>
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Un-snipping the QAPI schema change:

>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 0cf68fea14..bd651106bd 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1752,6 +1752,8 @@
>>  #
>>  # Get the named block driver list
>>  #
>> +# @flat: don't recurse into backing images if true. Default is false (Since 5.0)
>> +#
>>  # Returns: the list of BlockDeviceInfo
>>  #
>>  # Since: 2.0

What does it mean not to recurse?  Sounds like flat: false asks for a
subset of the full set.  How exactly is the subset defined?

>> @@ -1805,7 +1807,9 @@
>>  #                    } } ] }
>>  #
>>  ##
>> -{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
>> +{ 'command': 'query-named-block-nodes',
>> +  'returns': [ 'BlockDeviceInfo' ],
>> +  'data': { '*flat': 'bool' } }
>> 
>>  ##
>>  # @XDbgBlockGraphNodeType:
Eric Blake Dec. 17, 2019, 1:15 p.m. UTC | #3
On 12/17/19 1:36 AM, Markus Armbruster wrote:

> Un-snipping the QAPI schema change:

Sorry about that...

> 
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 0cf68fea14..bd651106bd 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1752,6 +1752,8 @@
>>>   #
>>>   # Get the named block driver list
>>>   #
>>> +# @flat: don't recurse into backing images if true. Default is false (Since 5.0)
>>> +#
>>>   # Returns: the list of BlockDeviceInfo
>>>   #
>>>   # Since: 2.0
> 
> What does it mean not to recurse?  Sounds like flat: false asks for a
> subset of the full set.  How exactly is the subset defined?

Bike-shedding:

Would it be easier to explain as:

@recurse: If true, include child information in each node (note that 
this can result in redundant output). Default is true (since 5.0)

and then pass false when you don't want recursion, with it being more 
obvious that using the new flag results in more compact output with no 
loss of information.
Markus Armbruster Dec. 17, 2019, 3:11 p.m. UTC | #4
Eric Blake <eblake@redhat.com> writes:

> On 12/17/19 1:36 AM, Markus Armbruster wrote:
>
>> Un-snipping the QAPI schema change:
>
> Sorry about that...
>
>>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 0cf68fea14..bd651106bd 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -1752,6 +1752,8 @@
>>>>   #
>>>>   # Get the named block driver list
>>>>   #
>>>> +# @flat: don't recurse into backing images if true. Default is false (Since 5.0)
>>>> +#
>>>>   # Returns: the list of BlockDeviceInfo
>>>>   #
>>>>   # Since: 2.0
>>
>> What does it mean not to recurse?  Sounds like flat: false asks for a
>> subset of the full set.  How exactly is the subset defined?
>
> Bike-shedding:
>
> Would it be easier to explain as:
>
> @recurse: If true, include child information in each node (note that
> this can result in redundant output). Default is true (since 5.0)
>
> and then pass false when you don't want recursion, with it being more
> obvious that using the new flag results in more compact output with no
> loss of information.

Adds a bit of information, namely that the information suppressed by
recurse: false is actually redundant.

The command's doc comment is weak: it doesn't really specify what
exactly is returned.  Simply omitting redundant information always
should still conform to this weak spec.  Of course, what really counts
isn't spec conformance, but meeting client expectations.  I don't even
understand what exactly gets returned, let alone how clients use it.

The doc comment needs to be judged by someone with actual knowledge in
this area.
Peter Krempa Dec. 19, 2019, 8:54 a.m. UTC | #5
On Tue, Dec 17, 2019 at 16:11:37 +0100, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 12/17/19 1:36 AM, Markus Armbruster wrote:
> >
> >> Un-snipping the QAPI schema change:
> >
> > Sorry about that...
> >
> >>
> >>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >>>> index 0cf68fea14..bd651106bd 100644
> >>>> --- a/qapi/block-core.json
> >>>> +++ b/qapi/block-core.json
> >>>> @@ -1752,6 +1752,8 @@
> >>>>   #
> >>>>   # Get the named block driver list
> >>>>   #
> >>>> +# @flat: don't recurse into backing images if true. Default is false (Since 5.0)
> >>>> +#
> >>>>   # Returns: the list of BlockDeviceInfo
> >>>>   #
> >>>>   # Since: 2.0
> >>
> >> What does it mean not to recurse?  Sounds like flat: false asks for a
> >> subset of the full set.  How exactly is the subset defined?
> >
> > Bike-shedding:
> >
> > Would it be easier to explain as:
> >
> > @recurse: If true, include child information in each node (note that
> > this can result in redundant output). Default is true (since 5.0)
> >
> > and then pass false when you don't want recursion, with it being more
> > obvious that using the new flag results in more compact output with no
> > loss of information.
> 
> Adds a bit of information, namely that the information suppressed by
> recurse: false is actually redundant.
> 
> The command's doc comment is weak: it doesn't really specify what
> exactly is returned.  Simply omitting redundant information always
> should still conform to this weak spec.  Of course, what really counts
> isn't spec conformance, but meeting client expectations.  I don't even
> understand what exactly gets returned, let alone how clients use it.

Well I by default expect that if the top level array has data for all
node names the nesting which repeats the information (partially) should
not be there because you can just look elsewhere for a more detailed
output.

Said that two years ago I wrote some code which detects the node names
from running qemu because at that time it was the only way to use the
block write threshold event. This code needs to extract the nesting
topology somehow but I don't remember nor fancy re-understanding the
algorithm for the detection during the hollidays. The only thing I can
say that the nesting is extracted either from the output of query-block
or from query-named-block nodes as both these outputs are fed to that
algorithm.

With blockdev that piece of code thankfully is unused, but unfortunately
I can't say that the nested output of query-named-block-nodes doesn't
have it's use.
diff mbox series

Patch

diff --git a/block.c b/block.c
index 473eb6eeaa..b30bdfa0d3 100644
--- a/block.c
+++ b/block.c
@@ -4766,14 +4766,15 @@  BlockDriverState *bdrv_find_node(const char *node_name)
 }

 /* Put this QMP function here so it can access the static graph_bdrv_states. */
-BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp)
+BlockDeviceInfoList *bdrv_named_nodes_list(bool flat,
+                                           Error **errp)
 {
     BlockDeviceInfoList *list, *entry;
     BlockDriverState *bs;

     list = NULL;
     QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
-        BlockDeviceInfo *info = bdrv_block_device_info(NULL, bs, errp);
+        BlockDeviceInfo *info = bdrv_block_device_info(NULL, bs, flat, errp);
         if (!info) {
             qapi_free_BlockDeviceInfoList(list);
             return NULL;
diff --git a/block/qapi.c b/block/qapi.c
index 9a5d0c9b27..84048e1a57 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -42,7 +42,9 @@ 
 #include "qemu/cutils.h"

 BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
-                                        BlockDriverState *bs, Error **errp)
+                                        BlockDriverState *bs,
+                                        bool flat,
+                                        Error **errp)
 {
     ImageInfo **p_image_info;
     BlockDriverState *bs0;
@@ -156,6 +158,10 @@  BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
             return NULL;
         }

+        /* stop gathering data for flat output */
+        if (flat)
+            break;
+
         if (bs0->drv && bs0->backing) {
             info->backing_file_depth++;
             bs0 = bs0->backing->bs;
@@ -389,7 +395,7 @@  static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,

     if (bs && bs->drv) {
         info->has_inserted = true;
-        info->inserted = bdrv_block_device_info(blk, bs, errp);
+        info->inserted = bdrv_block_device_info(blk, bs, false, errp);
         if (info->inserted == NULL) {
             goto err;
         }
diff --git a/blockdev.c b/blockdev.c
index 8e029e9c01..5f9c5e258f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3707,9 +3707,17 @@  void qmp_drive_backup(DriveBackup *arg, Error **errp)
     }
 }

-BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
+BlockDeviceInfoList *qmp_query_named_block_nodes(bool has_flat,
+                                                 bool flat,
+                                                 Error **errp)
 {
-    return bdrv_named_nodes_list(errp);
+    bool return_flat = false;
+
+    if (has_flat) {
+        return_flat = flat;
+    }
+
+    return bdrv_named_nodes_list(return_flat, errp);
 }

 XDbgBlockGraph *qmp_x_debug_query_block_graph(Error **errp)
diff --git a/include/block/block.h b/include/block/block.h
index 1df9848e74..177ba09e3f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -468,7 +468,7 @@  void bdrv_lock_medium(BlockDriverState *bs, bool locked);
 void bdrv_eject(BlockDriverState *bs, bool eject_flag);
 const char *bdrv_get_format_name(BlockDriverState *bs);
 BlockDriverState *bdrv_find_node(const char *node_name);
-BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp);
+BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, Error **errp);
 XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp);
 BlockDriverState *bdrv_lookup_bs(const char *device,
                                  const char *node_name,
diff --git a/include/block/qapi.h b/include/block/qapi.h
index cd9410dee3..22c7807c89 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -29,7 +29,9 @@ 
 #include "block/snapshot.h"

 BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
-                                        BlockDriverState *bs, Error **errp);
+                                        BlockDriverState *bs,
+                                        bool flat,
+                                        Error **errp);
 int bdrv_query_snapshot_info_list(BlockDriverState *bs,
                                   SnapshotInfoList **p_list,
                                   Error **errp);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b2551c16d1..651969819b 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -620,7 +620,7 @@  void hmp_info_block(Monitor *mon, const QDict *qdict)
     }

     /* Print node information */
-    blockdev_list = qmp_query_named_block_nodes(NULL);
+    blockdev_list = qmp_query_named_block_nodes(false, false, NULL);
     for (blockdev = blockdev_list; blockdev; blockdev = blockdev->next) {
         assert(blockdev->value->has_node_name);
         if (device && strcmp(device, blockdev->value->node_name)) {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0cf68fea14..bd651106bd 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1752,6 +1752,8 @@ 
 #
 # Get the named block driver list
 #
+# @flat: don't recurse into backing images if true. Default is false (Since 5.0)
+#
 # Returns: the list of BlockDeviceInfo
 #
 # Since: 2.0
@@ -1805,7 +1807,9 @@ 
 #                    } } ] }
 #
 ##
-{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
+{ 'command': 'query-named-block-nodes',
+  'returns': [ 'BlockDeviceInfo' ],
+  'data': { '*flat': 'bool' } }

 ##
 # @XDbgBlockGraphNodeType: