[Qemu-devel] [RFC for-2.7 0/1] block/qapi: Add query-block-node-tree

[Qemu-devel] [RFC for-2.7 0/1] block/qapi: Add query-block-node-tree

[Max Reitz]

As I responded to:
- http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg04464.html
- http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg05680.html

I think a general solution for querying the block node tree would be
nice (I don't think we actually have one). The single patch in this
series implements such a command.

However, this is an RFC because I'm not sure whether we really want this
and thus I didn't want to write the necessary tests for something we may
be going to discard anyway.

There are two reasons why I fear we may not want this:

The first is that the node graph is more or less something internal to
qemu. Its actual structure may (and most probably will) change over
time. We do want to be able to let the user or management application
manage the graph in fine detail, but these modifications are something
that can be emulated by legacy handling code later if we decide they are
no longer in line with the internal graph that we'd like to have.

However, if we emit the full graph with a command such as introduced
here, we can hardly change its outside appearance just to please legacy
applications. The output will change if the internal representation
changes.

I don't personally think this is too bad as long as we clearly state
this in the command's description: That qemu is free to implicitly
create intermediate nodes the user did not explicitly specify, and that
the set of nodes thus created may change over time.

No, I didn't specify this in this version, but it's an RFC, after all.
:-)


The second reason why I think we may not want this command: The known
unknowns. Everything that somehow allows the user to touch or see the
node graph may have some legacy implications later that right now I
fail to foresee.


And besides the known knowns and the known unknowns, there are of course
the unknown unknowns, but these exist with any patch.


In case you're wondering, a sample QMP invocation looks like this:

$ x86_64-softmmu/qemu-system-x86_64 \
  -drive if=none,id=drive,node-name=raw,driver=raw,\
         file.node-name=quorum,file.driver=quorum,\
         file.children.0.driver=null-co,\
         file.children.0.node-name=null0,\
         file.children.1.driver=null-co,\
         file.children.1.node-name=null1,\
         file.vote-threshold=1 \
         -qmp-pretty stdio -nodefaults
[...]
{'execute':'query-block-node-tree','arguments':{'root-node':'drive'}}
{
    "return": {
        "children": [
            {
                "role": "file",
                "node": {
                    "children": [
                        {
                            "role": "children.1",
                            "node": {
                                "children": [
                                ],
                                "node-name": "null1"
                            }
                        },
                        {
                            "role": "children.0",
                            "node": {
                                "children": [
                                ],
                                "node-name": "null0"
                            }
                        }
                    ],
                    "node-name": "quorum"
                }
            }
        ],
        "node-name": "raw"
    }
}


Max



Max Reitz (1):
  block/qapi: Add query-block-node-tree

 block/qapi.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx      | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+)

-- 
2.7.4

[Qemu-devel] [RFC for-2.7 1/1] block/qapi: Add query-block-node-tree

[Max Reitz]

This command returns the tree of BlockDriverStates under a given root
node.

Every tree node is described by its node name and the connection of a
parent node to its children additionally contains the role the child
assumes.

A node's name can then be used e.g. in conjunction with
query-named-block-nodes to get more information about the node.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qapi.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx      | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+)

diff --git a/block/qapi.c b/block/qapi.c
index 6a4869a..a35d32b 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -493,6 +493,49 @@ BlockInfoList *qmp_query_block(Error **errp)
     return head;
 }
 
+static BlockNodeTreeNode *qmp_query_block_node_tree_by_bs(BlockDriverState *bs)
+{
+    BlockNodeTreeNode *bntn;
+    BlockNodeTreeChildList **p_next;
+    BdrvChild *child;
+
+    bntn = g_new0(BlockNodeTreeNode, 1);
+
+    bntn->node_name = g_strdup(bdrv_get_node_name(bs));
+    bntn->has_node_name = bntn->node_name;
+
+    p_next = &bntn->children;
+    QLIST_FOREACH(child, &bs->children, next) {
+        BlockNodeTreeChild *bntc;
+
+        bntc = g_new(BlockNodeTreeChild, 1);
+        *bntc = (BlockNodeTreeChild){
+            .role = g_strdup(child->name),
+            .node = qmp_query_block_node_tree_by_bs(child->bs),
+        };
+
+        *p_next = g_new0(BlockNodeTreeChildList, 1);
+        (*p_next)->value = bntc;
+        p_next = &(*p_next)->next;
+    }
+
+    *p_next = NULL;
+    return bntn;
+}
+
+BlockNodeTreeNode *qmp_query_block_node_tree(const char *root_node,
+                                             Error **errp)
+{
+    BlockDriverState *bs;
+
+    bs = bdrv_lookup_bs(root_node, root_node, errp);
+    if (!bs) {
+        return NULL;
+    }
+
+    return qmp_query_block_node_tree_by_bs(bs);
+}
+
 static bool next_query_bds(BlockBackend **blk, BlockDriverState **bs,
                            bool query_nodes)
 {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b1cf77d..754ccd6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -470,6 +470,52 @@
 
 
 ##
+# @BlockNodeTreeNode:
+#
+# Describes a node in the block node graph.
+#
+# @node-name: If present, the node's name.
+#
+# @children:  List of the node's children.
+#
+# Since: 2.7
+##
+{ 'struct': 'BlockNodeTreeNode',
+  'data': { '*node-name': 'str',
+            'children': ['BlockNodeTreeChild'] } }
+
+##
+# @BlockNodeTreeChild:
+#
+# Describes a child node in the block node graph.
+#
+# @role: Role the child assumes for its parent, e.g. "file" or "backing".
+#
+# @node: The child node's BlockNodeTreeNode structure.
+#
+# Since: 2.7
+##
+{ 'struct': 'BlockNodeTreeChild',
+  'data': { 'role': 'str',
+            'node': 'BlockNodeTreeNode' } }
+
+##
+# @query-block-node-tree:
+#
+# Queries the tree of nodes under a given node in the block graph.
+#
+# @root-node: Node name or device name of the tree's root node.
+#
+# Returns: The root node's BlockNodeTreeNode structure.
+#
+# Since: 2.7
+##
+{ 'command': 'query-block-node-tree',
+  'data': { 'root-node': 'str' },
+  'returns': 'BlockNodeTreeNode' }
+
+
+##
 # @BlockDeviceTimedStats:
 #
 # Statistics of a block device during a given interval of time.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9e05365..5c404aa 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2637,6 +2637,44 @@ EQMP
     },
 
 SQMP
+query-block-node-tree
+---------------------
+
+Queries the tree of nodes under a given node in the block graph.
+
+Arguments:
+
+- "root-node": Node name or device name of the tree's root node (json-string)
+
+The block node tree is represented with BlockNodeTreeNode and BlockNodeTreeChild
+json-objects.
+
+Each BlockNodeTreeNode json-object contains the following:
+
+- "node-name": If present, the node's name (json-string, optional)
+- "children": json-array of the node's children, each entry is a json-object of
+              type BlockNodeTreeChild
+
+Each BlockNodeTreeChild json-object contains the following:
+
+- "role": Role the child node assumes for its parent, e.g. "file" or "backing"
+          (json-string)
+- "node": BlockNodeTreeNode describing the child node (json-object)
+
+The cyclic reference of BlockNodeTreeNode and BlockNodeTreeChild to each other
+thus spawns a tree.
+
+This command returns the root node's BlockNodeTreeNode structure.
+
+EQMP
+
+    {
+        .name       = "query-block-node-tree",
+        .args_type  = "root-node:B",
+        .mhandler.cmd_new = qmp_marshal_query_block_node_tree,
+    },
+
+SQMP
 query-blockstats
 ----------------
 
-- 
2.7.4

Re: [Qemu-devel] [RFC for-2.7 1/1] block/qapi: Add query-block-node-tree

[Wen Congyang]

On 03/25/2016 03:07 AM, Max Reitz wrote:
> This command returns the tree of BlockDriverStates under a given root
> node.
> 
> Every tree node is described by its node name and the connection of a
> parent node to its children additionally contains the role the child
> assumes.
> 
> A node's name can then be used e.g. in conjunction with
> query-named-block-nodes to get more information about the node.

I test this patch, and it works.
{'execute': 'query-block-node-tree', 'arguments': {'root-node': 'disk1' } }
{"return": {"children": [{"role": "children.0", "node": {"children": [{"role": "file", "node": {"children": [], "node-name": "#block175"}}], "node-name": "#block267"}}], "node-name": "#block040"}}

Shoule we hide the node name like "#blockxxx"?
If the bs doesn't have any child, should we output: '"children": [], '?

Can we add a new parameter: depth? For example, If I only want to know the quorum's
child name, we can limit the depth, and the output may be very clear.

Thanks
Wen Congyang

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qapi.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx      | 38 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 127 insertions(+)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 6a4869a..a35d32b 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -493,6 +493,49 @@ BlockInfoList *qmp_query_block(Error **errp)
>      return head;
>  }
>  
> +static BlockNodeTreeNode *qmp_query_block_node_tree_by_bs(BlockDriverState *bs)
> +{
> +    BlockNodeTreeNode *bntn;
> +    BlockNodeTreeChildList **p_next;
> +    BdrvChild *child;
> +
> +    bntn = g_new0(BlockNodeTreeNode, 1);
> +
> +    bntn->node_name = g_strdup(bdrv_get_node_name(bs));
> +    bntn->has_node_name = bntn->node_name;
> +
> +    p_next = &bntn->children;
> +    QLIST_FOREACH(child, &bs->children, next) {
> +        BlockNodeTreeChild *bntc;
> +
> +        bntc = g_new(BlockNodeTreeChild, 1);
> +        *bntc = (BlockNodeTreeChild){
> +            .role = g_strdup(child->name),
> +            .node = qmp_query_block_node_tree_by_bs(child->bs),
> +        };
> +
> +        *p_next = g_new0(BlockNodeTreeChildList, 1);
> +        (*p_next)->value = bntc;
> +        p_next = &(*p_next)->next;
> +    }
> +
> +    *p_next = NULL;
> +    return bntn;
> +}
> +
> +BlockNodeTreeNode *qmp_query_block_node_tree(const char *root_node,
> +                                             Error **errp)
> +{
> +    BlockDriverState *bs;
> +
> +    bs = bdrv_lookup_bs(root_node, root_node, errp);
> +    if (!bs) {
> +        return NULL;
> +    }
> +
> +    return qmp_query_block_node_tree_by_bs(bs);
> +}
> +
>  static bool next_query_bds(BlockBackend **blk, BlockDriverState **bs,
>                             bool query_nodes)
>  {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b1cf77d..754ccd6 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -470,6 +470,52 @@
>  
>  
>  ##
> +# @BlockNodeTreeNode:
> +#
> +# Describes a node in the block node graph.
> +#
> +# @node-name: If present, the node's name.
> +#
> +# @children:  List of the node's children.
> +#
> +# Since: 2.7
> +##
> +{ 'struct': 'BlockNodeTreeNode',
> +  'data': { '*node-name': 'str',
> +            'children': ['BlockNodeTreeChild'] } }
> +
> +##
> +# @BlockNodeTreeChild:
> +#
> +# Describes a child node in the block node graph.
> +#
> +# @role: Role the child assumes for its parent, e.g. "file" or "backing".
> +#
> +# @node: The child node's BlockNodeTreeNode structure.
> +#
> +# Since: 2.7
> +##
> +{ 'struct': 'BlockNodeTreeChild',
> +  'data': { 'role': 'str',
> +            'node': 'BlockNodeTreeNode' } }
> +
> +##
> +# @query-block-node-tree:
> +#
> +# Queries the tree of nodes under a given node in the block graph.
> +#
> +# @root-node: Node name or device name of the tree's root node.
> +#
> +# Returns: The root node's BlockNodeTreeNode structure.
> +#
> +# Since: 2.7
> +##
> +{ 'command': 'query-block-node-tree',
> +  'data': { 'root-node': 'str' },
> +  'returns': 'BlockNodeTreeNode' }
> +
> +
> +##
>  # @BlockDeviceTimedStats:
>  #
>  # Statistics of a block device during a given interval of time.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 9e05365..5c404aa 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2637,6 +2637,44 @@ EQMP
>      },
>  
>  SQMP
> +query-block-node-tree
> +---------------------
> +
> +Queries the tree of nodes under a given node in the block graph.
> +
> +Arguments:
> +
> +- "root-node": Node name or device name of the tree's root node (json-string)
> +
> +The block node tree is represented with BlockNodeTreeNode and BlockNodeTreeChild
> +json-objects.
> +
> +Each BlockNodeTreeNode json-object contains the following:
> +
> +- "node-name": If present, the node's name (json-string, optional)
> +- "children": json-array of the node's children, each entry is a json-object of
> +              type BlockNodeTreeChild
> +
> +Each BlockNodeTreeChild json-object contains the following:
> +
> +- "role": Role the child node assumes for its parent, e.g. "file" or "backing"
> +          (json-string)
> +- "node": BlockNodeTreeNode describing the child node (json-object)
> +
> +The cyclic reference of BlockNodeTreeNode and BlockNodeTreeChild to each other
> +thus spawns a tree.
> +
> +This command returns the root node's BlockNodeTreeNode structure.
> +
> +EQMP
> +
> +    {
> +        .name       = "query-block-node-tree",
> +        .args_type  = "root-node:B",
> +        .mhandler.cmd_new = qmp_marshal_query_block_node_tree,
> +    },
> +
> +SQMP
>  query-blockstats
>  ----------------
>  
> 

Re: [Qemu-devel] [RFC for-2.7 1/1] block/qapi: Add query-block-node-tree

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2011 bytes --]

On 25.03.2016 03:50, Wen Congyang wrote:
> On 03/25/2016 03:07 AM, Max Reitz wrote:
>> This command returns the tree of BlockDriverStates under a given root
>> node.
>>
>> Every tree node is described by its node name and the connection of a
>> parent node to its children additionally contains the role the child
>> assumes.
>>
>> A node's name can then be used e.g. in conjunction with
>> query-named-block-nodes to get more information about the node.
> 
> I test this patch, and it works.
> {'execute': 'query-block-node-tree', 'arguments': {'root-node': 'disk1' } }
> {"return": {"children": [{"role": "children.0", "node": {"children": [{"role": "file", "node": {"children": [], "node-name": "#block175"}}], "node-name": "#block267"}}], "node-name": "#block040"}}
> 
> Shoule we hide the node name like "#blockxxx"?

No, I don't think so. In fact I was thinking of making the node name
non-optional because every BDS should have one due to these
auto-generated ones.

> If the bs doesn't have any child, should we output: '"children": [], '?

Omitting an empty array (thus making the @children key optional)
occurred to me, too. I didn't find any strong reason to do so, however.
It makes generating the output a bit more complicated and may also make
parsing it just a tiny bit more complicated.

I think that omitting it would make more sense to a human reader, but
QMP is a machine protocol, so this is not a very strong argument.

So all in all I saw neither strong arguments to omit an empty array nor
to include it. Thus I included it because that makes the code simpler.

> Can we add a new parameter: depth? For example, If I only want to know the quorum's
> child name, we can limit the depth, and the output may be very clear.

Good idea. I can do so in the next version, but first I'd like to hear
more opinions on what other people think of this command. If they think
it's fine, I'll include a depth parameter.

Thank you for you comments!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [RFC for-2.7 1/1] block/qapi: Add query-block-node-tree

[Wen Congyang]

On 03/25/2016 03:07 AM, Max Reitz wrote:
> This command returns the tree of BlockDriverStates under a given root
> node.
> 
> Every tree node is described by its node name and the connection of a
> parent node to its children additionally contains the role the child
> assumes.
> 
> A node's name can then be used e.g. in conjunction with
> query-named-block-nodes to get more information about the node.

I found another problem:

{'execute': 'query-block-node-tree', 'arguments': {'root-node': 'disk1' } }
{"return": {"children": [{"role": "children.1", "node": {"children": [{"role": "file", "node": {}}], "node-name": "test1"}}, {"role": "children.0", "node": {"children": [{"role": "file", "node": {}}]}}]}}

s->children[0] is children.0, and s->children[1] is children.1.
But we output them in reverse order. The reason is:

BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
                             BlockDriverState *child_bs,
                             const char *child_name,
                             const BdrvChildRole *child_role)
{
    BdrvChild *child = bdrv_root_attach_child(child_bs, child_name, child_role);
    QLIST_INSERT_HEAD(&parent_bs->children, child, next);
    return child;
}

We insert the new child to the head, not the tail...

Thanks
Wen Congyang

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qapi.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx      | 38 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 127 insertions(+)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 6a4869a..a35d32b 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -493,6 +493,49 @@ BlockInfoList *qmp_query_block(Error **errp)
>      return head;
>  }
>  
> +static BlockNodeTreeNode *qmp_query_block_node_tree_by_bs(BlockDriverState *bs)
> +{
> +    BlockNodeTreeNode *bntn;
> +    BlockNodeTreeChildList **p_next;
> +    BdrvChild *child;
> +
> +    bntn = g_new0(BlockNodeTreeNode, 1);
> +
> +    bntn->node_name = g_strdup(bdrv_get_node_name(bs));
> +    bntn->has_node_name = bntn->node_name;
> +
> +    p_next = &bntn->children;
> +    QLIST_FOREACH(child, &bs->children, next) {
> +        BlockNodeTreeChild *bntc;
> +
> +        bntc = g_new(BlockNodeTreeChild, 1);
> +        *bntc = (BlockNodeTreeChild){
> +            .role = g_strdup(child->name),
> +            .node = qmp_query_block_node_tree_by_bs(child->bs),
> +        };
> +
> +        *p_next = g_new0(BlockNodeTreeChildList, 1);
> +        (*p_next)->value = bntc;
> +        p_next = &(*p_next)->next;
> +    }
> +
> +    *p_next = NULL;
> +    return bntn;
> +}
> +
> +BlockNodeTreeNode *qmp_query_block_node_tree(const char *root_node,
> +                                             Error **errp)
> +{
> +    BlockDriverState *bs;
> +
> +    bs = bdrv_lookup_bs(root_node, root_node, errp);
> +    if (!bs) {
> +        return NULL;
> +    }
> +
> +    return qmp_query_block_node_tree_by_bs(bs);
> +}
> +
>  static bool next_query_bds(BlockBackend **blk, BlockDriverState **bs,
>                             bool query_nodes)
>  {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b1cf77d..754ccd6 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -470,6 +470,52 @@
>  
>  
>  ##
> +# @BlockNodeTreeNode:
> +#
> +# Describes a node in the block node graph.
> +#
> +# @node-name: If present, the node's name.
> +#
> +# @children:  List of the node's children.
> +#
> +# Since: 2.7
> +##
> +{ 'struct': 'BlockNodeTreeNode',
> +  'data': { '*node-name': 'str',
> +            'children': ['BlockNodeTreeChild'] } }
> +
> +##
> +# @BlockNodeTreeChild:
> +#
> +# Describes a child node in the block node graph.
> +#
> +# @role: Role the child assumes for its parent, e.g. "file" or "backing".
> +#
> +# @node: The child node's BlockNodeTreeNode structure.
> +#
> +# Since: 2.7
> +##
> +{ 'struct': 'BlockNodeTreeChild',
> +  'data': { 'role': 'str',
> +            'node': 'BlockNodeTreeNode' } }
> +
> +##
> +# @query-block-node-tree:
> +#
> +# Queries the tree of nodes under a given node in the block graph.
> +#
> +# @root-node: Node name or device name of the tree's root node.
> +#
> +# Returns: The root node's BlockNodeTreeNode structure.
> +#
> +# Since: 2.7
> +##
> +{ 'command': 'query-block-node-tree',
> +  'data': { 'root-node': 'str' },
> +  'returns': 'BlockNodeTreeNode' }
> +
> +
> +##
>  # @BlockDeviceTimedStats:
>  #
>  # Statistics of a block device during a given interval of time.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 9e05365..5c404aa 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2637,6 +2637,44 @@ EQMP
>      },
>  
>  SQMP
> +query-block-node-tree
> +---------------------
> +
> +Queries the tree of nodes under a given node in the block graph.
> +
> +Arguments:
> +
> +- "root-node": Node name or device name of the tree's root node (json-string)
> +
> +The block node tree is represented with BlockNodeTreeNode and BlockNodeTreeChild
> +json-objects.
> +
> +Each BlockNodeTreeNode json-object contains the following:
> +
> +- "node-name": If present, the node's name (json-string, optional)
> +- "children": json-array of the node's children, each entry is a json-object of
> +              type BlockNodeTreeChild
> +
> +Each BlockNodeTreeChild json-object contains the following:
> +
> +- "role": Role the child node assumes for its parent, e.g. "file" or "backing"
> +          (json-string)
> +- "node": BlockNodeTreeNode describing the child node (json-object)
> +
> +The cyclic reference of BlockNodeTreeNode and BlockNodeTreeChild to each other
> +thus spawns a tree.
> +
> +This command returns the root node's BlockNodeTreeNode structure.
> +
> +EQMP
> +
> +    {
> +        .name       = "query-block-node-tree",
> +        .args_type  = "root-node:B",
> +        .mhandler.cmd_new = qmp_marshal_query_block_node_tree,
> +    },
> +
> +SQMP
>  query-blockstats
>  ----------------
>  
> 

Re: [Qemu-devel] [RFC for-2.7 1/1] block/qapi: Add query-block-node-tree

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2026 bytes --]

On 25.03.2016 07:54, Wen Congyang wrote:
> On 03/25/2016 03:07 AM, Max Reitz wrote:
>> This command returns the tree of BlockDriverStates under a given root
>> node.
>>
>> Every tree node is described by its node name and the connection of a
>> parent node to its children additionally contains the role the child
>> assumes.
>>
>> A node's name can then be used e.g. in conjunction with
>> query-named-block-nodes to get more information about the node.
> 
> I found another problem:
> 
> {'execute': 'query-block-node-tree', 'arguments': {'root-node': 'disk1' } }
> {"return": {"children": [{"role": "children.1", "node": {"children": [{"role": "file", "node": {}}], "node-name": "test1"}}, {"role": "children.0", "node": {"children": [{"role": "file", "node": {}}]}}]}}
> 
> s->children[0] is children.0, and s->children[1] is children.1.
> But we output them in reverse order. The reason is:
> 
> BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>                              BlockDriverState *child_bs,
>                              const char *child_name,
>                              const BdrvChildRole *child_role)
> {
>     BdrvChild *child = bdrv_root_attach_child(child_bs, child_name, child_role);
>     QLIST_INSERT_HEAD(&parent_bs->children, child, next);
>     return child;
> }
> 
> We insert the new child to the head, not the tail...

Well, the idea is that the order of children doesn't really matter; The
only thing that describes the behavior of a child is its role. For
instance, for qcow2 it doesn't matter whether the "file" or the
"backing" BDS is the first child.

However, for quorum, the order might matter (e.g. in FIFO mode). But
then again, the order is clearly specified by the role again: The first
child is the one with the "children.0" role.

So I don't think this is real problem as long as I add a note to the
documentation that the order of objects in the @children array is
undefined and does not have any significance.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [RFC for-2.7 1/1] block/qapi: Add query-block-node-tree

[Eric Blake]

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

On 03/26/2016 10:33 AM, Max Reitz wrote:

>> We insert the new child to the head, not the tail...
> 
> Well, the idea is that the order of children doesn't really matter; The
> only thing that describes the behavior of a child is its role. For
> instance, for qcow2 it doesn't matter whether the "file" or the
> "backing" BDS is the first child.
> 
> However, for quorum, the order might matter (e.g. in FIFO mode). But
> then again, the order is clearly specified by the role again: The first
> child is the one with the "children.0" role.
> 
> So I don't think this is real problem as long as I add a note to the
> documentation that the order of objects in the @children array is
> undefined and does not have any significance.

What happens when we hot-add/-delete children from a FIFO mode quorum?
Obviously, we can select which child to delete, so if we delete the
child.0 node, do we have guaranteed semantics on which node takes over
the role as the primary child?  Conversely, when adding a node, do we
have a way to specify whether the new node must be at the front (assume
the child.0 role) or back (must not affect the current FIFO roles)?

I think order is important enough for FIFO mode that we ought to try and
represent it explicitly in the command, rather than getting it backwards
or stating it is undefined.

-- 
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] [RFC for-2.7 1/1] block/qapi: Add query-block-node-tree

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3230 bytes --]

On 28.03.2016 17:25, Eric Blake wrote:
> On 03/26/2016 10:33 AM, Max Reitz wrote:
> 
>>> We insert the new child to the head, not the tail...
>>
>> Well, the idea is that the order of children doesn't really matter; The
>> only thing that describes the behavior of a child is its role. For
>> instance, for qcow2 it doesn't matter whether the "file" or the
>> "backing" BDS is the first child.
>>
>> However, for quorum, the order might matter (e.g. in FIFO mode). But
>> then again, the order is clearly specified by the role again: The first
>> child is the one with the "children.0" role.
>>
>> So I don't think this is real problem as long as I add a note to the
>> documentation that the order of objects in the @children array is
>> undefined and does not have any significance.
> 
> What happens when we hot-add/-delete children from a FIFO mode quorum?

Good question. I'd have hoped it'd be children.1 (because that is then
the first child).

But it appears to be more or less a random child. It just always
iterates through the s->children array (from start to finish), but when
children.0 is deleted, that array is compacted so the first entry is
then indeed children.1; but when children.0 is added again, it will be
appended to the end of the array (and so you can bring that array into a
random order, basically).

However, I don't think that we should expose this array's order to the
outside because of this, but that it is a bug that should be addressed
in a new version of the hot-add/-delete series.

> Obviously, we can select which child to delete, so if we delete the
> child.0 node, do we have guaranteed semantics on which node takes over
> the role as the primary child?  Conversely, when adding a node, do we
> have a way to specify whether the new node must be at the front (assume
> the child.0 role) or back (must not affect the current FIFO roles)?

No, we don't. I'm not sure whether we need this, but in any case it's
something the hot-add/-delete series needs to address.

> I think order is important enough for FIFO mode that we ought to try and
> represent it explicitly in the command, rather than getting it backwards
> or stating it is undefined.

In my opinion, the way the order is explicitly represented is through
every child's role. For quorum, "children.${i}" comes before
"children.${i+1}".

The general block layer does not care about these generic children, it
only cares about "file" and "backing". Therefore, it cannot know the
order of the children (if there is any) and it in fact does not care. If
there is an order, we would thus need to get the block driver to define
it and I don't think that's trivial (the easiest way to do so probably
is to define a driver-supplied iterator function).

Note that any order of children would be driver-specific still, just as
generic children's role names are driver-specific. Therefore, if a user
knows how to interpret the order of children, they'd know how to derive
the order from the role name, too.

Also noteworthy is that it's completely fine to leave the order
undefined for now and implement functionality to sort the array at some
later point in time.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [RFC for-2.7 1/1] block/qapi: Add query-block-node-tree

[Eric Blake]

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

On 03/29/2016 09:29 AM, Max Reitz wrote:

> 
> In my opinion, the way the order is explicitly represented is through
> every child's role. For quorum, "children.${i}" comes before
> "children.${i+1}".
> 
> The general block layer does not care about these generic children, it
> only cares about "file" and "backing". Therefore, it cannot know the
> order of the children (if there is any) and it in fact does not care. If
> there is an order, we would thus need to get the block driver to define
> it and I don't think that's trivial (the easiest way to do so probably
> is to define a driver-supplied iterator function).
> 
> Note that any order of children would be driver-specific still, just as
> generic children's role names are driver-specific. Therefore, if a user
> knows how to interpret the order of children, they'd know how to derive
> the order from the role name, too.

That argument is reasonable - either a callback so that a driver can
emit children in the order it desires, or else the documentation that if
order matters, the user must be able to reconstruct order based on
information already present without having to rely on the array being sored.

> 
> Also noteworthy is that it's completely fine to leave the order
> undefined for now and implement functionality to sort the array at some
> later point in time.

That's harder - it's not easy to introspect whether output will be
sorted or not, so clients would always have to treat the data as
unsorted, at which point adding sorting later doesn't help.  Sorting is
only useful to add up front, where you can document it as part of the
contract; so now the question is whether sorting is useful enough to
worry about making it part of the contract.

-- 
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] [RFC for-2.7 1/1] block/qapi: Add query-block-node-tree

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2310 bytes --]

On 29.03.2016 17:39, Eric Blake wrote:
> On 03/29/2016 09:29 AM, Max Reitz wrote:
> 
>>
>> In my opinion, the way the order is explicitly represented is through
>> every child's role. For quorum, "children.${i}" comes before
>> "children.${i+1}".
>>
>> The general block layer does not care about these generic children, it
>> only cares about "file" and "backing". Therefore, it cannot know the
>> order of the children (if there is any) and it in fact does not care. If
>> there is an order, we would thus need to get the block driver to define
>> it and I don't think that's trivial (the easiest way to do so probably
>> is to define a driver-supplied iterator function).
>>
>> Note that any order of children would be driver-specific still, just as
>> generic children's role names are driver-specific. Therefore, if a user
>> knows how to interpret the order of children, they'd know how to derive
>> the order from the role name, too.
> 
> That argument is reasonable - either a callback so that a driver can
> emit children in the order it desires, or else the documentation that if
> order matters, the user must be able to reconstruct order based on
> information already present without having to rely on the array being sored.
> 
>>
>> Also noteworthy is that it's completely fine to leave the order
>> undefined for now and implement functionality to sort the array at some
>> later point in time.
> 
> That's harder - it's not easy to introspect whether output will be
> sorted or not, so clients would always have to treat the data as
> unsorted, at which point adding sorting later doesn't help.  Sorting is
> only useful to add up front, where you can document it as part of the
> contract; so now the question is whether sorting is useful enough to
> worry about making it part of the contract.

Well, we can make it introspectable, it will just be a bit ugly. For
instance, just adding a "sorted" boolean would solve that issue. It's
ugly but it's not as if it would actually hurt anybody.

The only result would be that we should not return a BlockNodeTreeNode
directly but a more complex structure which then contains the root
BlockNodeTreeNode and may contain more information about the tree in the
future (e.g. the "sorted" boolean).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [RFC for-2.7 0/1] block/qapi: Add query-block-node-tree

[Kevin Wolf]

Am 24.03.2016 um 20:07 hat Max Reitz geschrieben:
> As I responded to:
> - http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg04464.html
> - http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg05680.html
> 
> I think a general solution for querying the block node tree would be
> nice (I don't think we actually have one). The single patch in this
> series implements such a command.
> 
> However, this is an RFC because I'm not sure whether we really want this
> and thus I didn't want to write the necessary tests for something we may
> be going to discard anyway.

I think we do want to have a way to query the tree structure in QMP. The
part that I'm so sure about is whether we want a recursive command or
one that just covers the children of a single node. If we want the
latter, just adding a children field (which maps role names to node
names) to query-block might be enough.

Kevin

Re: [Qemu-devel] [RFC for-2.7 0/1] block/qapi: Add query-block-node-tree

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1487 bytes --]

On 29.03.2016 17:51, Kevin Wolf wrote:
> Am 24.03.2016 um 20:07 hat Max Reitz geschrieben:
>> As I responded to:
>> - http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg04464.html
>> - http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg05680.html
>>
>> I think a general solution for querying the block node tree would be
>> nice (I don't think we actually have one). The single patch in this
>> series implements such a command.
>>
>> However, this is an RFC because I'm not sure whether we really want this
>> and thus I didn't want to write the necessary tests for something we may
>> be going to discard anyway.
> 
> I think we do want to have a way to query the tree structure in QMP. The
> part that I'm so sure about is whether we want a recursive command or
> one that just covers the children of a single node. If we want the
> latter, just adding a children field (which maps role names to node
> names) to query-block might be enough.

Sounds fine in principle to me, but I'd like to add that I think we may
want to have a new command for querying a specific BDS. You don't get
much choice which nodes you query with query-block, and
query-named-block-nodes seems a bit bloated to me by now...

About the "mapping role names to node names" though; I suppose you mean
like { "file": "file-node", "backing": "other-qcow2-node" }. I'd have
loved to do that, but I couldn't imagine a way to represent that in the
QAPI schema.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [RFC for-2.7 0/1] block/qapi: Add query-block-node-tree

[Kevin Wolf]

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

Am 29.03.2016 um 17:56 hat Max Reitz geschrieben:
> On 29.03.2016 17:51, Kevin Wolf wrote:
> > Am 24.03.2016 um 20:07 hat Max Reitz geschrieben:
> >> As I responded to:
> >> - http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg04464.html
> >> - http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg05680.html
> >>
> >> I think a general solution for querying the block node tree would be
> >> nice (I don't think we actually have one). The single patch in this
> >> series implements such a command.
> >>
> >> However, this is an RFC because I'm not sure whether we really want this
> >> and thus I didn't want to write the necessary tests for something we may
> >> be going to discard anyway.
> > 
> > I think we do want to have a way to query the tree structure in QMP. The
> > part that I'm so sure about is whether we want a recursive command or
> > one that just covers the children of a single node. If we want the
> > latter, just adding a children field (which maps role names to node
> > names) to query-block might be enough.
> 
> Sounds fine in principle to me, but I'd like to add that I think we may
> want to have a new command for querying a specific BDS. You don't get
> much choice which nodes you query with query-block, and
> query-named-block-nodes seems a bit bloated to me by now...

Maybe we need to start over with a new query-block-node command that
takes a node name (or BB name as an alias for its root node) and returns
information only about that node. That is, it adds node-name support
compared to query-block, but it removes the recursive crap for backing
files that we added there.

> About the "mapping role names to node names" though; I suppose you mean
> like { "file": "file-node", "backing": "other-qcow2-node" }. I'd have
> loved to do that, but I couldn't imagine a way to represent that in the
> QAPI schema.

Eric and Markus will tell us, but if nothing else helps:

    { 'children': 'any' }

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [RFC for-2.7 0/1] block/qapi: Add query-block-node-tree

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2124 bytes --]

On 29.03.2016 18:09, Kevin Wolf wrote:
> Am 29.03.2016 um 17:56 hat Max Reitz geschrieben:
>> On 29.03.2016 17:51, Kevin Wolf wrote:
>>> Am 24.03.2016 um 20:07 hat Max Reitz geschrieben:
>>>> As I responded to:
>>>> - http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg04464.html
>>>> - http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg05680.html
>>>>
>>>> I think a general solution for querying the block node tree would be
>>>> nice (I don't think we actually have one). The single patch in this
>>>> series implements such a command.
>>>>
>>>> However, this is an RFC because I'm not sure whether we really want this
>>>> and thus I didn't want to write the necessary tests for something we may
>>>> be going to discard anyway.
>>>
>>> I think we do want to have a way to query the tree structure in QMP. The
>>> part that I'm so sure about is whether we want a recursive command or
>>> one that just covers the children of a single node. If we want the
>>> latter, just adding a children field (which maps role names to node
>>> names) to query-block might be enough.
>>
>> Sounds fine in principle to me, but I'd like to add that I think we may
>> want to have a new command for querying a specific BDS. You don't get
>> much choice which nodes you query with query-block, and
>> query-named-block-nodes seems a bit bloated to me by now...
> 
> Maybe we need to start over with a new query-block-node command that
> takes a node name (or BB name as an alias for its root node) and returns
> information only about that node. That is, it adds node-name support
> compared to query-block, but it removes the recursive crap for backing
> files that we added there.

Yes, sounds good to me.

>> About the "mapping role names to node names" though; I suppose you mean
>> like { "file": "file-node", "backing": "other-qcow2-node" }. I'd have
>> loved to do that, but I couldn't imagine a way to represent that in the
>> QAPI schema.
> 
> Eric and Markus will tell us, but if nothing else helps:
> 
>     { 'children': 'any' }

But that's cheating! :-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [RFC for-2.7 0/1] block/qapi: Add query-block-node-tree

[Alberto Garcia]

On Thu 24 Mar 2016 08:07:17 PM CET, Max Reitz wrote:
> There are two reasons why I fear we may not want this:
>
> The first is that the node graph is more or less something internal to
> qemu. Its actual structure may (and most probably will) change over
> time. We do want to be able to let the user or management application
> manage the graph in fine detail, but these modifications are something
> that can be emulated by legacy handling code later if we decide they are
> no longer in line with the internal graph that we'd like to have.
>
> However, if we emit the full graph with a command such as introduced
> here, we can hardly change its outside appearance just to please legacy
> applications. The output will change if the internal representation
> changes.
>
> I don't personally think this is too bad as long as we clearly state
> this in the command's description: That qemu is free to implicitly
> create intermediate nodes the user did not explicitly specify, and that
> the set of nodes thus created may change over time.

...but if the internal representation can change over time, then the
user cannot rely on the output of this command, so what's the use?
Stating that in the command description wouldn't solve anything.

Berto

Re: [Qemu-devel] [Qemu-block] [RFC for-2.7 0/1] block/qapi: Add query-block-node-tree

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2046 bytes --]

On 30.03.2016 14:43, Alberto Garcia wrote:
> On Thu 24 Mar 2016 08:07:17 PM CET, Max Reitz wrote:
>> There are two reasons why I fear we may not want this:
>>
>> The first is that the node graph is more or less something internal to
>> qemu. Its actual structure may (and most probably will) change over
>> time. We do want to be able to let the user or management application
>> manage the graph in fine detail, but these modifications are something
>> that can be emulated by legacy handling code later if we decide they are
>> no longer in line with the internal graph that we'd like to have.
>>
>> However, if we emit the full graph with a command such as introduced
>> here, we can hardly change its outside appearance just to please legacy
>> applications. The output will change if the internal representation
>> changes.
>>
>> I don't personally think this is too bad as long as we clearly state
>> this in the command's description: That qemu is free to implicitly
>> create intermediate nodes the user did not explicitly specify, and that
>> the set of nodes thus created may change over time.
> 
> ...but if the internal representation can change over time, then the
> user cannot rely on the output of this command, so what's the use?
> Stating that in the command description wouldn't solve anything.

That depends. If we know that qemu will never implicitly add anything
but single-parent-single-children filters into the chain and we state
so, that is something users can work around: If they encounter an
unknown node, they just skip down to its child.

Alternatively, we could define that for unknown nodes a user wants to
skip, they can walk down to the "file" child.

So if we state "We do with the graph whatever we want and you get no
guarantees whatsoever", that is indeed not very helpful. But if we can
give some guarantees, i.e. that qemu will never implicitly remove
user-defined nodes and how to behave in case an unknown node is
encountered, then it can still be of use.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [RFC for-2.7 0/1] block/qapi: Add query-block-node-tree

[Stefan Hajnoczi]

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

On Thu, Mar 24, 2016 at 08:07:17PM +0100, Max Reitz wrote:
> As I responded to:
> - http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg04464.html
> - http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg05680.html
> 
> I think a general solution for querying the block node tree would be
> nice (I don't think we actually have one). The single patch in this
> series implements such a command.
> 
> However, this is an RFC because I'm not sure whether we really want this
> and thus I didn't want to write the necessary tests for something we may
> be going to discard anyway.
> 
> There are two reasons why I fear we may not want this:
> 
> The first is that the node graph is more or less something internal to
> qemu. Its actual structure may (and most probably will) change over
> time. We do want to be able to let the user or management application
> manage the graph in fine detail, but these modifications are something
> that can be emulated by legacy handling code later if we decide they are
> no longer in line with the internal graph that we'd like to have.
> 
> However, if we emit the full graph with a command such as introduced
> here, we can hardly change its outside appearance just to please legacy
> applications. The output will change if the internal representation
> changes.
> 
> I don't personally think this is too bad as long as we clearly state
> this in the command's description: That qemu is free to implicitly
> create intermediate nodes the user did not explicitly specify, and that
> the set of nodes thus created may change over time.
> 
> No, I didn't specify this in this version, but it's an RFC, after all.
> :-)

I'm also concerned about exposing the graph since QEMU may implement
features with internal block filters (I/O throttling, backup block job
to get rid of write notifiers, etc).  These nodes come and go depending
on high-level commands issued by the guest *and* are dependent on the
QEMU version.

What is the purpose of this command - I didn't see that in your cover
letter?  Does it still serve its purpose if we warn the user that the
graph structure can contain little surprises :)?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [RFC for-2.7 0/1] block/qapi: Add query-block-node-tree

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3260 bytes --]

On 31.03.2016 11:49, Stefan Hajnoczi wrote:
> On Thu, Mar 24, 2016 at 08:07:17PM +0100, Max Reitz wrote:
>> As I responded to:
>> - http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg04464.html
>> - http://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg05680.html
>>
>> I think a general solution for querying the block node tree would be
>> nice (I don't think we actually have one). The single patch in this
>> series implements such a command.
>>
>> However, this is an RFC because I'm not sure whether we really want this
>> and thus I didn't want to write the necessary tests for something we may
>> be going to discard anyway.
>>
>> There are two reasons why I fear we may not want this:
>>
>> The first is that the node graph is more or less something internal to
>> qemu. Its actual structure may (and most probably will) change over
>> time. We do want to be able to let the user or management application
>> manage the graph in fine detail, but these modifications are something
>> that can be emulated by legacy handling code later if we decide they are
>> no longer in line with the internal graph that we'd like to have.
>>
>> However, if we emit the full graph with a command such as introduced
>> here, we can hardly change its outside appearance just to please legacy
>> applications. The output will change if the internal representation
>> changes.
>>
>> I don't personally think this is too bad as long as we clearly state
>> this in the command's description: That qemu is free to implicitly
>> create intermediate nodes the user did not explicitly specify, and that
>> the set of nodes thus created may change over time.
>>
>> No, I didn't specify this in this version, but it's an RFC, after all.
>> :-)
> 
> I'm also concerned about exposing the graph since QEMU may implement
> features with internal block filters (I/O throttling, backup block job
> to get rid of write notifiers, etc).  These nodes come and go depending
> on high-level commands issued by the guest *and* are dependent on the
> QEMU version.
> 
> What is the purpose of this command - I didn't see that in your cover
> letter?

It's partially in the links I gave above, and partially because I think
such a command would just be nice to have.

The mails linked above concern themselves with getting a list of
children of a quorum node, so I responded that a more general solution
would be nicer.

>          Does it still serve its purpose if we warn the user that the
> graph structure can contain little surprises :)?

As I replied to Berto, I think we can come up with some constraints
about what qemu may do and what it will not do. That way, we will keep
the flexibility we need and users can still get the information they want.

For instance, I proposed that qemu will never remove explicitly created
nodes but may always implicitly add nodes. If a user thus encounters an
unknown and/or unexpected node, it should just skip the node and fall
through to its "file" child.

I think that fact that implicitly created nodes will always be inserted
into edges in the BDS graph such that the edge's child will be the
node's "file" child is something we are able to guarantee.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [RFC for-2.7 0/1] block/qapi: Add query-block-node-tree

[Stefan Hajnoczi]

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

On Fri, Apr 01, 2016 at 05:30:58PM +0200, Max Reitz wrote:
> >          Does it still serve its purpose if we warn the user that the
> > graph structure can contain little surprises :)?
> 
> As I replied to Berto, I think we can come up with some constraints
> about what qemu may do and what it will not do. That way, we will keep
> the flexibility we need and users can still get the information they want.
> 
> For instance, I proposed that qemu will never remove explicitly created
> nodes but may always implicitly add nodes. If a user thus encounters an
> unknown and/or unexpected node, it should just skip the node and fall
> through to its "file" child.
> 
> I think that fact that implicitly created nodes will always be inserted
> into edges in the BDS graph such that the edge's child will be the
> node's "file" child is something we are able to guarantee.

Another option would be for nodes to have a child alias that dumb
clients use when skipping the node.  This way we're not tied to actually
implementing it with bs->file.

One thing we must be careful about is to avoid using parent->child
relationships in QMP APIs because they introduce race conditions:

Imagine a block job that inserts a filter node.  If the client wishes to
insert its own node below the filter node we must solve the following
race condition.  The client queries the block graph and traverses to
find the parent node.  Now it issues a QMP command to insert its node
below the parent.  If the block job happens to complete right before the
QMP command is processed, there will be an error because the parent node
no longer exists.

The race conditions must be kept in mind for all APIs we design once we
begin exposing the block graph :(.

I would say it's an error for the client to refer to internal nodes.
QEMU shouldn't allow the client to name internal nodes due to the race
condition issue.  Perhaps they shouldn't have an externally visible node
ID/name at all.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]