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

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(+)

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

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

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

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

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.

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

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.

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