diff mbox series

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

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

Commit Message

Peter Krempa Jan. 20, 2020, 8:50 a.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>
---

Diff to v1:
 - rewrote setting of 'return_flat' in qmp_query_named_block_nodes
 - tried to clarify the QMP schema docs for the new field

This patch does not aim to fix the rather suboptimal original
documentation of the command as that is going to end up in a bunch of
bikeshedding.

While I know that there are plans for a new command that should fix
this, the plans were already there for quite some time without much
happening. This is a quick fix to a real problem, because if you have
(maybe unpractically) deep backing chains, the returned JSON is getting
huge. (140 nesting levels exceeds 10MiB of JSON)

 block.c               |  5 +++--
 block/qapi.c          | 10 ++++++++--
 blockdev.c            |  8 ++++++--
 include/block/block.h |  2 +-
 include/block/qapi.h  |  4 +++-
 monitor/hmp-cmds.c    |  2 +-
 qapi/block-core.json  |  7 ++++++-
 7 files changed, 28 insertions(+), 10 deletions(-)

Comments

Eric Blake Jan. 20, 2020, 7:16 p.m. UTC | #1
On 1/20/20 2:50 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.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
> 
> Diff to v1:
>   - rewrote setting of 'return_flat' in qmp_query_named_block_nodes
>   - tried to clarify the QMP schema docs for the new field
> 
> This patch does not aim to fix the rather suboptimal original
> documentation of the command as that is going to end up in a bunch of
> bikeshedding.
> 
> While I know that there are plans for a new command that should fix
> this, the plans were already there for quite some time without much
> happening. This is a quick fix to a real problem, because if you have
> (maybe unpractically) deep backing chains, the returned JSON is getting
> huge. (140 nesting levels exceeds 10MiB of JSON)

Yep, O(n^2) output growth based on a depth of N is not ideal.

> +++ b/qapi/block-core.json
> @@ -1752,6 +1752,9 @@
>   #
>   # Get the named block driver list
>   #
> +# @flat: Omit the nested data about backing image ("backing-image" key) if true.
> +#        Default is false (Since 5.0)
> +#
>   # Returns: the list of BlockDeviceInfo
>   #
>   # Since: 2.0
> @@ -1805,7 +1808,9 @@
>   #                    } } ] }
>   #
>   ##
> -{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
> +{ 'command': 'query-named-block-nodes',
> +  'returns': [ 'BlockDeviceInfo' ],
> +  'data': { '*flat': 'bool' } }

Reviewed-by: Eric Blake <eblake@redhat.com>
Max Reitz Feb. 7, 2020, 10:08 a.m. UTC | #2
On 20.01.20 09:50, 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.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
> 
> Diff to v1:
>  - rewrote setting of 'return_flat' in qmp_query_named_block_nodes
>  - tried to clarify the QMP schema docs for the new field
> 
> This patch does not aim to fix the rather suboptimal original
> documentation of the command as that is going to end up in a bunch of
> bikeshedding.
> 
> While I know that there are plans for a new command that should fix
> this, the plans were already there for quite some time without much
> happening. This is a quick fix to a real problem, because if you have
> (maybe unpractically) deep backing chains, the returned JSON is getting
> huge. (140 nesting levels exceeds 10MiB of JSON)

The main reason nothing is happening is because nobody is pressing for
it, I think.  We talk about it from time to time but then our result is
“As long as nobody seriously complains and tells us what we need, we’re
going to assume what we have is good enough.”

For example:
https://lists.nongnu.org/archive/html/qemu-block/2020-01/msg00049.html
(Under “Query function situation”)

>  block.c               |  5 +++--
>  block/qapi.c          | 10 ++++++++--
>  blockdev.c            |  8 ++++++--
>  include/block/block.h |  2 +-
>  include/block/qapi.h  |  4 +++-
>  monitor/hmp-cmds.c    |  2 +-
>  qapi/block-core.json  |  7 ++++++-
>  7 files changed, 28 insertions(+), 10 deletions(-)

[...]

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

This should be enclosed in curly brackets (qemu coding style).

Shall I fix that up?

Max

> +
>          if (bs0->drv && bs0->backing) {
>              info->backing_file_depth++;
>              bs0 = bs0->backing->bs;
diff mbox series

Patch

diff --git a/block.c b/block.c
index ecd09dbbfd..73d70aec11 100644
--- a/block.c
+++ b/block.c
@@ -4784,14 +4784,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..cba2f567aa 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3707,9 +3707,13 @@  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 = has_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 e9dcfef7fa..afced5249c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -469,7 +469,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 d0e0af893a..bece8447a5 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -619,7 +619,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 7ff5e5edaf..56561ff703 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1752,6 +1752,9 @@ 
 #
 # Get the named block driver list
 #
+# @flat: Omit the nested data about backing image ("backing-image" key) if true.
+#        Default is false (Since 5.0)
+#
 # Returns: the list of BlockDeviceInfo
 #
 # Since: 2.0
@@ -1805,7 +1808,9 @@ 
 #                    } } ] }
 #
 ##
-{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
+{ 'command': 'query-named-block-nodes',
+  'returns': [ 'BlockDeviceInfo' ],
+  'data': { '*flat': 'bool' } }

 ##
 # @XDbgBlockGraphNodeType: