[Qemu-devel] [PATCH v2 0/3] block nodes graph visualization

[Qemu-devel] [PATCH v2 0/3] block nodes graph visualization

[Vladimir Sementsov-Ogievskiy]

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

Hi all!

On the way of backup schemes development (and in general any complicated
developments in Qemu block layer) it would be good to have an ability to 
print
out graph of block nodes with their permissions. Just look at attached 
picture.

v2: major rework: Identifying non-bds nodes by their description was a 
bad idea,
descriptions are not guaranteed to be different for different nodes. So, 
the only
way is use pointer to identify them. And to be unique, let's use pointers to
identify all the nodes in the graph. As additional benefit, we have 
pointers for
each node, which is good for debugging (imagine a gdb session).

Vladimir Sementsov-Ogievskiy (3):
   qapi: add x-query-block-graph
   scripts: add render_block_graph function for QEMUMachine
   not-for-commit: example of new command usage for debugging

  qapi/block-core.json          | 116 
++++++++++++++++++++++++++++++++++++++++++
  include/block/block.h         |   1 +
  block.c                       |  80 +++++++++++++++++++++++++++++
  blockdev.c                    |   5 ++
  scripts/render_block_graph.py |  78 ++++++++++++++++++++++++++++
  tests/qemu-iotests/222        |   3 ++
  6 files changed, 283 insertions(+)
  create mode 100644 scripts/render_block_graph.py

-- 
2.11.1


[-- Attachment #2: out.dot.png --]
[-- Type: image/png, Size: 75363 bytes --]

[-- Attachment #3: out.pointers.dot.png --]
[-- Type: image/png, Size: 89913 bytes --]

[Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Vladimir Sementsov-Ogievskiy]

Add a new command, returning block nodes graph.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |   1 +
 block.c               |  80 ++++++++++++++++++++++++++++++++++
 blockdev.c            |   5 +++
 4 files changed, 202 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4c7a37afdc..32398c9912 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1630,6 +1630,122 @@
 { 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
 
 ##
+# @BlockGraphNodeType:
+#
+# @block: Actual block nodes, which may be queried by query-named-block-nodes.
+# @other: Not a block node. Examples: guest block device, block job.
+#
+# Since: 3.1
+##
+{ 'enum': 'BlockGraphNodeType',
+  'data': [ 'block', 'other' ] }
+
+##
+# @BlockGraphNodeTypeBlock:
+#
+# Since: 3.1
+##
+{ 'struct': 'BlockGraphNodeTypeBlock',
+  'data': { 'node-name': 'str' } }
+
+##
+# @BlockGraphNodeTypeOther:
+#
+# Since: 3.1
+##
+  { 'struct': 'BlockGraphNodeTypeOther',
+    'data': { 'description': 'str' } }
+
+##
+# @BlockGraphNode:
+#
+# @id: unique node identifier, equal to node RAM-pointer
+# @type: node type
+#
+# Since: 3.1
+##
+{ 'union': 'BlockGraphNode',
+  'base': { 'id': 'uint64', 'type': 'BlockGraphNodeType' },
+  'discriminator': 'type',
+  'data' : { 'block': 'BlockGraphNodeTypeBlock',
+             'other': 'BlockGraphNodeTypeOther' } }
+
+##
+# @BlockPermission:
+#
+# Enum of base block permissions.
+#
+# @consistent-read: A user that has the "permission" of consistent reads is
+#                   guaranteed that their view of the contents of the block
+#                   device is complete and self-consistent, representing the
+#                   contents of a disk at a specific point.
+#                   For most block devices (including their backing files) this
+#                   is true, but the property cannot be maintained in a few
+#                   situations like for intermediate nodes of a commit block
+#                   job.
+#
+# @write: This permission is required to change the visible disk contents.
+#
+# @write-unchanged: This permission (which is weaker than BLK_PERM_WRITE) is
+#                   both enough and required for writes to the block node when
+#                   the caller promises that the visible disk content doesn't
+#                   change.
+#                   As the BLK_PERM_WRITE permission is strictly stronger,
+#                   either is sufficient to perform an unchanging write.
+#
+# @resize: This permission is required to change the size of a block node.
+#
+# @graph-mod: This permission is required to change the node that this
+#             BdrvChild points to.
+#
+# Since: 3.1
+##
+  { 'enum': 'BlockPermission',
+    'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize',
+              'graph-mod' ] }
+##
+# @BlockGraphEdge:
+#
+# Block Graph edge description for x-query-block-graph.
+#
+# @parent: parent id
+#
+# @child: child id
+#
+# @name: name of the relation (examples are 'file' and 'backing')
+#
+# @perm: granted permissions for the parent operating on the child
+#
+# @shared-perm: permissions that can still be granted to other users of the
+#               child while it is still attached this parent
+#
+# Since: 3.1
+##
+{ 'struct': 'BlockGraphEdge',
+  'data': { 'parent': 'uint64', 'child': 'uint64',
+            'name': 'str', 'perm': [ 'BlockPermission' ],
+            'shared-perm': [ 'BlockPermission' ] } }
+
+##
+# @BlockGraph:
+#
+# Block Graph - list of nodes and list of edges.
+#
+# Since: 3.1
+##
+{ 'struct': 'BlockGraph',
+  'data': { 'nodes': ['BlockGraphNode'], 'edges': ['BlockGraphEdge'] } }
+
+##
+# @x-query-block-graph:
+#
+# Get the block graph.
+#
+# Since: 3.1
+##
+{ 'command': 'x-query-block-graph', 'returns': 'BlockGraph' }
+
+##
 # @drive-mirror:
 #
 # Start mirroring a block device's writes to a new destination. target
diff --git a/include/block/block.h b/include/block/block.h
index 4e0871aaf9..6f2ccad040 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -448,6 +448,7 @@ 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);
+BlockGraph *bdrv_get_block_graph(Error **errp);
 BlockDriverState *bdrv_lookup_bs(const char *device,
                                  const char *node_name,
                                  Error **errp);
diff --git a/block.c b/block.c
index 6161dbe3eb..766354e79c 100644
--- a/block.c
+++ b/block.c
@@ -4003,6 +4003,86 @@ BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp)
     return list;
 }
 
+#define QAPI_LIST_ADD(list, element) do { \
+    typeof(list) _tmp = g_new(typeof(*(list)), 1); \
+    _tmp->value = (element); \
+    _tmp->next = (list); \
+    list = _tmp; \
+} while (0)
+
+BlockGraph *bdrv_get_block_graph(Error **errp)
+{
+    BlockGraph *graph = g_new0(BlockGraph, 1);
+    BlockDriverState *bs;
+    BdrvChild *child;
+    BlockGraphNode *node;
+    BlockGraphEdge *edge;
+    struct {
+        unsigned int flag;
+        BlockPermission num;
+    } permissions[] = {
+        { BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ },
+        { BLK_PERM_WRITE,           BLOCK_PERMISSION_WRITE },
+        { BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED },
+        { BLK_PERM_RESIZE,          BLOCK_PERMISSION_RESIZE },
+        { BLK_PERM_GRAPH_MOD,       BLOCK_PERMISSION_GRAPH_MOD },
+        { 0, 0 }
+    }, *p;
+
+    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
+        QLIST_FOREACH(child, &bs->parents, next_parent) {
+            if (!child->role->parent_is_bds) {
+                BlockGraphNodeList *el;
+                bool add = true;
+
+                for (el = graph->nodes; el; el = el->next) {
+                    if (el->value->id == (uint64_t)child->opaque) {
+                        add = false;
+                        break;
+                    }
+                }
+
+                if (add) {
+                    node = g_new0(BlockGraphNode, 1);
+                    node->id = (uint64_t)child->opaque;
+                    node->type = BLOCK_GRAPH_NODE_TYPE_OTHER;
+                    node->u.other.description =
+                            g_strdup(bdrv_child_user_desc(child));
+                    QAPI_LIST_ADD(graph->nodes, node);
+                }
+            }
+
+            edge = g_new0(BlockGraphEdge, 1);
+
+            edge->parent = (uint64_t)child->opaque;
+            edge->child = (uint64_t)bs;
+            assert(bs == child->bs);
+            edge->name = g_strdup(child->name);
+
+            for (p = permissions; p->flag; p++) {
+                if (p->flag & child->perm) {
+                    QAPI_LIST_ADD(edge->perm, p->num);
+                }
+                if (p->flag & child->shared_perm) {
+                    QAPI_LIST_ADD(edge->shared_perm, p->num);
+                }
+            }
+
+            QAPI_LIST_ADD(graph->edges, edge);
+        }
+    }
+
+    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
+        node = g_new0(BlockGraphNode, 1);
+        node->id = (uint64_t)bs;
+        node->type = BLOCK_GRAPH_NODE_TYPE_BLOCK;
+        node->u.block.node_name = g_strdup(bdrv_get_node_name(bs));
+        QAPI_LIST_ADD(graph->nodes, node);
+    }
+
+    return graph;
+}
+
 BlockDriverState *bdrv_lookup_bs(const char *device,
                                  const char *node_name,
                                  Error **errp)
diff --git a/blockdev.c b/blockdev.c
index 72f5347df5..099a27d7b8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3485,6 +3485,11 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
     return bdrv_named_nodes_list(errp);
 }
 
+BlockGraph *qmp_x_query_block_graph(Error **errp)
+{
+    return bdrv_get_block_graph(errp);
+}
+
 BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
                              Error **errp)
 {
-- 
2.11.1

[Qemu-devel] [PATCH v2 2/3] scripts: add render_block_graph function for QEMUMachine

[Vladimir Sementsov-Ogievskiy]

Render block nodes graph with help of graphviz. This new function is
for debugging, so there is no sense to put it into qemu.py as a method
of QEMUMachine. Let's instead put it separately.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/render_block_graph.py | 78 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 scripts/render_block_graph.py

diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
new file mode 100644
index 0000000000..7048a0bac8
--- /dev/null
+++ b/scripts/render_block_graph.py
@@ -0,0 +1,78 @@
+# Render Qemu Block Graph
+#
+# Copyright (c) 2017 Parallels International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+from graphviz import Digraph
+
+def perm(arr):
+    s = 'w' if 'write' in arr else '_'
+    s += 'r' if 'consistent-read' in arr else '_'
+    s += 'u' if 'write-unchanged' in arr else '_'
+    s += 'g' if 'graph-mod' in arr else '_'
+    s += 's' if 'resize' in arr else '_'
+    return s
+
+def render_block_graph(vm, filename, pointers=False, format='png'):
+    '''
+    Render graph in text (dot) representation into "@filename" and
+    representation in @format into "@filename.@format"
+    '''
+
+    nodes = vm.command('query-named-block-nodes')
+    nodes_info = {n['node-name']: n for n in nodes}
+
+    block_graph = vm.command('x-query-block-graph')
+
+    graph = Digraph(comment='Block Nodes Graph')
+    graph.format = format
+    graph.node('permission symbols:\l'
+               '  w - Write\l'
+               '  r - consistent-Read\l'
+               '  u - write - Unchanged\l'
+               '  g - Graph-mod\l'
+               '  s - reSize\l'
+               'edge label scheme:\l'
+               '  <child type>\l'
+               '  <perm>\l'
+               '  <shared_perm>\l', shape='none')
+
+
+    for n in block_graph['nodes']:
+        if n['type'] == 'block':
+            info = nodes_info[n['node-name']]
+            label = n['node-name'] + ' [' + info['drv'] + ']'
+            shape = 'ellipse'
+        else:
+            assert n['type'] == 'other'
+            label = n['description']
+            shape = 'box'
+
+        if pointers:
+            label += '\n0x%x' % n['id']
+
+        if n['type'] == 'block' and info['drv'] == 'file':
+            label += '\n' + os.path.basename(info['file'])
+
+        graph.node(str(n['id']), label, shape=shape)
+
+    for e in block_graph['edges']:
+        label = '%s\l%s\l%s\l' % (e['name'], perm(e['perm']),
+                                  perm(e['shared-perm']))
+        graph.edge(str(e['parent']), str(e['child']), label=label)
+
+    graph.render(filename)
-- 
2.11.1

[Qemu-devel] [PATCH v2 3/3] not-for-commit: example of new command usage for debugging

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/222 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
index 0ead56d574..c2e647fa83 100644
--- a/tests/qemu-iotests/222
+++ b/tests/qemu-iotests/222
@@ -137,6 +137,9 @@ with iotests.FilePath('base.img') as base_img_path, \
     log('--- Cleanup ---')
     log('')
 
+    from render_block_graph import render_block_graph
+    render_block_graph(vm, '/tmp/out.dot')
+    render_block_graph(vm, '/tmp/out.pointers.dot', pointers=True)
     log(vm.qmp('block-job-cancel', device=src_node))
     log(vm.event_wait('BLOCK_JOB_CANCELLED'),
         filters=[iotests.filter_qmp_event])
-- 
2.11.1

Re: [Qemu-devel] [PATCH v2 2/3] scripts: add render_block_graph function for QEMUMachine

[Eduardo Habkost]

On Fri, Aug 17, 2018 at 09:04:39PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Render block nodes graph with help of graphviz. This new function is
> for debugging, so there is no sense to put it into qemu.py as a method
> of QEMUMachine. Let's instead put it separately.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  scripts/render_block_graph.py | 78 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
>  create mode 100644 scripts/render_block_graph.py
> 
> diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
> new file mode 100644
> index 0000000000..7048a0bac8
> --- /dev/null
> +++ b/scripts/render_block_graph.py
> @@ -0,0 +1,78 @@
> +# Render Qemu Block Graph
[...]

What about making the script work from the command-line?

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
old mode 100644
new mode 100755
index 7048a0bac8..e29fe0fc41
--- a/scripts/render_block_graph.py
+++ b/scripts/render_block_graph.py
@@ -1,3 +1,4 @@
+#!/usr/bin/env python
 # Render Qemu Block Graph
 #
 # Copyright (c) 2017 Parallels International GmbH
@@ -16,8 +17,9 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
-import os
+import os, sys
 from graphviz import Digraph
+from qmp.qmp import QEMUMonitorProtocol
 
 def perm(arr):
     s = 'w' if 'write' in arr else '_'
@@ -27,16 +29,16 @@ def perm(arr):
     s += 's' if 'resize' in arr else '_'
     return s
 
-def render_block_graph(vm, filename, pointers=False, format='png'):
+def render_block_graph(qmp, filename, pointers=False, format='png'):
     '''
     Render graph in text (dot) representation into "@filename" and
     representation in @format into "@filename.@format"
     '''
 
-    nodes = vm.command('query-named-block-nodes')
+    nodes = qmp.command('query-named-block-nodes')
     nodes_info = {n['node-name']: n for n in nodes}
 
-    block_graph = vm.command('x-query-block-graph')
+    block_graph = qmp.command('x-query-block-graph')
 
     graph = Digraph(comment='Block Nodes Graph')
     graph.format = format
@@ -76,3 +78,9 @@ def render_block_graph(vm, filename, pointers=False, format='png'):
         graph.edge(str(e['parent']), str(e['child']), label=label)
 
     graph.render(filename)
+
+if __name__ == '__main__':
+    #TODO: use argparse for command-line arguments
+    qmp = QEMUMonitorProtocol(sys.argv[1])
+    qmp.connect()
+    render_block_graph(qmp, sys.argv[2])


-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 2/3] scripts: add render_block_graph function for QEMUMachine

[Vladimir Sementsov-Ogievskiy]

17.08.2018 21:25, Eduardo Habkost wrote:
> On Fri, Aug 17, 2018 at 09:04:39PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Render block nodes graph with help of graphviz. This new function is
>> for debugging, so there is no sense to put it into qemu.py as a method
>> of QEMUMachine. Let's instead put it separately.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   scripts/render_block_graph.py | 78 +++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 78 insertions(+)
>>   create mode 100644 scripts/render_block_graph.py
>>
>> diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
>> new file mode 100644
>> index 0000000000..7048a0bac8
>> --- /dev/null
>> +++ b/scripts/render_block_graph.py
>> @@ -0,0 +1,78 @@
>> +# Render Qemu Block Graph
> [...]
>
> What about making the script work from the command-line?
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
> old mode 100644
> new mode 100755
> index 7048a0bac8..e29fe0fc41
> --- a/scripts/render_block_graph.py
> +++ b/scripts/render_block_graph.py
> @@ -1,3 +1,4 @@
> +#!/usr/bin/env python
>   # Render Qemu Block Graph
>   #
>   # Copyright (c) 2017 Parallels International GmbH
> @@ -16,8 +17,9 @@
>   # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   #
>   
> -import os
> +import os, sys
>   from graphviz import Digraph
> +from qmp.qmp import QEMUMonitorProtocol
>   
>   def perm(arr):
>       s = 'w' if 'write' in arr else '_'
> @@ -27,16 +29,16 @@ def perm(arr):
>       s += 's' if 'resize' in arr else '_'
>       return s
>   
> -def render_block_graph(vm, filename, pointers=False, format='png'):
> +def render_block_graph(qmp, filename, pointers=False, format='png'):
>       '''
>       Render graph in text (dot) representation into "@filename" and
>       representation in @format into "@filename.@format"
>       '''
>   
> -    nodes = vm.command('query-named-block-nodes')
> +    nodes = qmp.command('query-named-block-nodes')
>       nodes_info = {n['node-name']: n for n in nodes}
>   
> -    block_graph = vm.command('x-query-block-graph')
> +    block_graph = qmp.command('x-query-block-graph')
>   
>       graph = Digraph(comment='Block Nodes Graph')
>       graph.format = format
> @@ -76,3 +78,9 @@ def render_block_graph(vm, filename, pointers=False, format='png'):
>           graph.edge(str(e['parent']), str(e['child']), label=label)
>   
>       graph.render(filename)
> +
> +if __name__ == '__main__':
> +    #TODO: use argparse for command-line arguments
> +    qmp = QEMUMonitorProtocol(sys.argv[1])
> +    qmp.connect()
> +    render_block_graph(qmp, sys.argv[2])
>
>

Cool, thanks.

so, how to use it then?

for python iotest a proper parameter would be vm._qmp, yes?

is there a way to dump running libvirt vm graph?

I've started libvirt guest, qemu process has cmdline parameters
-chardev 
socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-3-tmp/monitor.sock,server,nowait
-mon chardev=charmonitor,id=monitor,mode=control

command
virsh qemu-monitor-command tmp '{"execute": "x-query-block-graph"}'
works fine,

but script hangs on connection:
# python ./scripts/render_block_graph.py 
/var/lib/libvirt/qemu/domain-3-tmp/monitor.sock
^CTraceback (most recent call last):
   File "./scripts/render_block_graph.py", line 85, in <module>
     qmp.connect()
   File "/work/src/qemu/up-new-fleecing/scripts/qmp/qmp.py", line 140, 
in connect
     self.__sock.connect(self.__address)
   File "/usr/lib64/python2.7/socket.py", line 224, in meth
     return getattr(self._sock,name)(*args)
KeyboardInterrupt

./scripts/qmp/qmp-shell /var/lib/libvirt/qemu/domain-3-tmp/monitor.sock
outputs nothing...


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 2/3] scripts: add render_block_graph function for QEMUMachine

[Eduardo Habkost]

On Fri, Aug 17, 2018 at 09:59:41PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 17.08.2018 21:25, Eduardo Habkost wrote:
> > On Fri, Aug 17, 2018 at 09:04:39PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Render block nodes graph with help of graphviz. This new function is
> > > for debugging, so there is no sense to put it into qemu.py as a method
> > > of QEMUMachine. Let's instead put it separately.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > ---
> > >   scripts/render_block_graph.py | 78 +++++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 78 insertions(+)
> > >   create mode 100644 scripts/render_block_graph.py
> > > 
> > > diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
> > > new file mode 100644
> > > index 0000000000..7048a0bac8
> > > --- /dev/null
> > > +++ b/scripts/render_block_graph.py
> > > @@ -0,0 +1,78 @@
> > > +# Render Qemu Block Graph
> > [...]
> > 
> > What about making the script work from the command-line?
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
> > old mode 100644
> > new mode 100755
> > index 7048a0bac8..e29fe0fc41
> > --- a/scripts/render_block_graph.py
> > +++ b/scripts/render_block_graph.py
> > @@ -1,3 +1,4 @@
> > +#!/usr/bin/env python
> >   # Render Qemu Block Graph
> >   #
> >   # Copyright (c) 2017 Parallels International GmbH
> > @@ -16,8 +17,9 @@
> >   # along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >   #
> > -import os
> > +import os, sys
> >   from graphviz import Digraph
> > +from qmp.qmp import QEMUMonitorProtocol
> >   def perm(arr):
> >       s = 'w' if 'write' in arr else '_'
> > @@ -27,16 +29,16 @@ def perm(arr):
> >       s += 's' if 'resize' in arr else '_'
> >       return s
> > -def render_block_graph(vm, filename, pointers=False, format='png'):
> > +def render_block_graph(qmp, filename, pointers=False, format='png'):
> >       '''
> >       Render graph in text (dot) representation into "@filename" and
> >       representation in @format into "@filename.@format"
> >       '''
> > -    nodes = vm.command('query-named-block-nodes')
> > +    nodes = qmp.command('query-named-block-nodes')
> >       nodes_info = {n['node-name']: n for n in nodes}
> > -    block_graph = vm.command('x-query-block-graph')
> > +    block_graph = qmp.command('x-query-block-graph')
> >       graph = Digraph(comment='Block Nodes Graph')
> >       graph.format = format
> > @@ -76,3 +78,9 @@ def render_block_graph(vm, filename, pointers=False, format='png'):
> >           graph.edge(str(e['parent']), str(e['child']), label=label)
> >       graph.render(filename)
> > +
> > +if __name__ == '__main__':
> > +    #TODO: use argparse for command-line arguments
> > +    qmp = QEMUMonitorProtocol(sys.argv[1])
> > +    qmp.connect()
> > +    render_block_graph(qmp, sys.argv[2])
> > 
> > 
> 
> Cool, thanks.
> 
> so, how to use it then?
> 
> for python iotest a proper parameter would be vm._qmp, yes?

Yes, assuming you just want to do it locally just for debugging.

One thing we could do later: refactor QEMUMachine and
QEMUMonitorProtocol so their method names are the same and both
'vm' and 'vm._qmp' work here.

> 
> is there a way to dump running libvirt vm graph?
> 
> I've started libvirt guest, qemu process has cmdline parameters
> -chardev socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-3-tmp/monitor.sock,server,nowait
> -mon chardev=charmonitor,id=monitor,mode=control
> 
> command
> virsh qemu-monitor-command tmp '{"execute": "x-query-block-graph"}'
> works fine,
> 
> but script hangs on connection:
> # python ./scripts/render_block_graph.py
> /var/lib/libvirt/qemu/domain-3-tmp/monitor.sock
> ^CTraceback (most recent call last):
>   File "./scripts/render_block_graph.py", line 85, in <module>
>     qmp.connect()
>   File "/work/src/qemu/up-new-fleecing/scripts/qmp/qmp.py", line 140, in
> connect
>     self.__sock.connect(self.__address)
>   File "/usr/lib64/python2.7/socket.py", line 224, in meth
>     return getattr(self._sock,name)(*args)
> KeyboardInterrupt
> 
> ./scripts/qmp/qmp-shell /var/lib/libvirt/qemu/domain-3-tmp/monitor.sock
> outputs nothing...

I don't know why it doesn't work with a running instance started
by libvirt, but it works if I run QEMU manually using "-qmp
unix:/tmp/qmp,server".

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Eric Blake]

On 08/17/2018 01:04 PM, Vladimir Sementsov-Ogievskiy wrote:
> Add a new command, returning block nodes graph.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/block-core.json  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++

> +##
> +# @BlockGraphEdge:
> +#
> +# Block Graph edge description for x-query-block-graph.
> +#
> +# @parent: parent id
> +#
> +# @child: child id
> +#
> +# @name: name of the relation (examples are 'file' and 'backing')

Can this be made into a QAPI enum? (It would have ripple effects to 
existing code, but might be a worthwhile cleanup).

> +#
> +# @perm: granted permissions for the parent operating on the child
> +#
> +# @shared-perm: permissions that can still be granted to other users of the
> +#               child while it is still attached this parent
> +#
> +# Since: 3.1
> +##
> +{ 'struct': 'BlockGraphEdge',
> +  'data': { 'parent': 'uint64', 'child': 'uint64',
> +            'name': 'str', 'perm': [ 'BlockPermission' ],
> +            'shared-perm': [ 'BlockPermission' ] } }
> +

> +##
> +# @x-query-block-graph:
> +#
> +# Get the block graph.
> +#
> +# Since: 3.1
> +##
> +{ 'command': 'x-query-block-graph', 'returns': 'BlockGraph' }

Why is this command given an x- prefix?  What would it take to promote 
it from experimental to fully supported?

Overall, the command looks quite useful; and the fact that you produced 
some nice .dot graphs from it for visualization seems like it is worth 
considering this as a permanent API addition.  The question, then, is if 
the interface is right, or if it needs any tweaks (like using an enum 
instead of open-coded string for the relation between parent and child), 
as a reason for keeping the x- prefix.

> +++ b/block.c
> @@ -4003,6 +4003,86 @@ BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp)
>       return list;
>   }
>   
> +#define QAPI_LIST_ADD(list, element) do { \
> +    typeof(list) _tmp = g_new(typeof(*(list)), 1); \
> +    _tmp->value = (element); \
> +    _tmp->next = (list); \
> +    list = _tmp; \
> +} while (0)

Hmm - this seems like a frequently observed pattern - should it be 
something that we add independently and use throughout the tree as part 
of QAPI interactions in general?

> +
> +BlockGraph *bdrv_get_block_graph(Error **errp)
> +{
> +    BlockGraph *graph = g_new0(BlockGraph, 1);
> +    BlockDriverState *bs;
> +    BdrvChild *child;
> +    BlockGraphNode *node;
> +    BlockGraphEdge *edge;
> +    struct {
> +        unsigned int flag;
> +        BlockPermission num;
> +    } permissions[] = {
> +        { BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ },

Would it be worth directly reusing a QAPI enum for all such permissions 
in the existing code base, instead of having to map between an internal 
and a QAPI enum?

> +        { BLK_PERM_WRITE,           BLOCK_PERMISSION_WRITE },
> +        { BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED },
> +        { BLK_PERM_RESIZE,          BLOCK_PERMISSION_RESIZE },
> +        { BLK_PERM_GRAPH_MOD,       BLOCK_PERMISSION_GRAPH_MOD },
> +        { 0, 0 }
> +    }, *p;
> +
> +    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
> +        QLIST_FOREACH(child, &bs->parents, next_parent) {
> +            if (!child->role->parent_is_bds) {
> +                BlockGraphNodeList *el;
> +                bool add = true;

Does your command need/want to expose internal nodes? (I argue that it 
probably should show internal nodes, even when normal 'query-block' 
doesn't, just because knowing about the impact of internal nodes can be 
important when tracking down permissions issues).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Max Reitz]

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

On 2018-08-17 22:32, Eric Blake wrote:
> On 08/17/2018 01:04 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Add a new command, returning block nodes graph.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json  | 116
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
> 
>> +##
>> +# @BlockGraphEdge:
>> +#
>> +# Block Graph edge description for x-query-block-graph.
>> +#
>> +# @parent: parent id
>> +#
>> +# @child: child id
>> +#
>> +# @name: name of the relation (examples are 'file' and 'backing')
> 
> Can this be made into a QAPI enum? (It would have ripple effects to
> existing code, but might be a worthwhile cleanup).
> 
>> +#
>> +# @perm: granted permissions for the parent operating on the child
>> +#
>> +# @shared-perm: permissions that can still be granted to other users
>> of the
>> +#               child while it is still attached this parent
>> +#
>> +# Since: 3.1
>> +##
>> +{ 'struct': 'BlockGraphEdge',
>> +  'data': { 'parent': 'uint64', 'child': 'uint64',
>> +            'name': 'str', 'perm': [ 'BlockPermission' ],
>> +            'shared-perm': [ 'BlockPermission' ] } }
>> +
> 
>> +##
>> +# @x-query-block-graph:
>> +#
>> +# Get the block graph.
>> +#
>> +# Since: 3.1
>> +##
>> +{ 'command': 'x-query-block-graph', 'returns': 'BlockGraph' }
> 
> Why is this command given an x- prefix?  What would it take to promote
> it from experimental to fully supported?

This is just a very bad reasons, but I'll give it anyway: We really want
such a command but still don't have one.  So I doubt this is exactly
what we want. :-)

A better reason is that we probably do not want a single command to
return the whole block graph.  Do we want the command to just return
info for a single node, including just the node names of the children?
Do we want the command to include child info on request (but start from
a user-specified root)?

Also, the interface is...  Er, weird?  Honestly, I don't quite see why
we would want it like this without x-.

Why use newly generated IDs instead of node names?  Why are those RAM
addresses?  That is just so fishy.

Because parents can be non-nodes?  Well, I suppose you better not
include them in the graph like this, then.

I don't see why the query command we want would include non-BDSs at all.

It may be useful for debugging, so, er, well, with an x-debug- prefix,
OK.  But the question then is whether it's useful enough to warrant
having a separate query command that isn't precisely the command we want
anyway.

The first question we probably have to ask is whether the query command
needs to output parent information.  If not, querying would probably
start at some named node and then you'd go down from there (either with
many queries or through a single one).

If so, well, then we can still output parent information, but I'd say
that then is purely informational and we don't need to "generate" new
IDs for them.  Just have either a node-name there or a user-readable
description (like it was in v1; although it has to be noted that such a
user-readable description is useless to a management layer), but these
new IDs are really not something I like.

> Overall, the command looks quite useful; and the fact that you produced
> some nice .dot graphs from it for visualization seems like it is worth
> considering this as a permanent API addition.  The question, then, is if
> the interface is right, or if it needs any tweaks (like using an enum
> instead of open-coded string for the relation between parent and child),
> as a reason for keeping the x- prefix.

You can use x-debug- even when you decide to keep a command.

I see no reason why a command should hastily not get an x- prefix just
because it may be useful enough.  If it really is and we really see the
interface is good (which I really don't think it is), then we can always
drop it later.

Max

>> +++ b/block.c
>> @@ -4003,6 +4003,86 @@ BlockDeviceInfoList
>> *bdrv_named_nodes_list(Error **errp)
>>       return list;
>>   }
>>   +#define QAPI_LIST_ADD(list, element) do { \
>> +    typeof(list) _tmp = g_new(typeof(*(list)), 1); \
>> +    _tmp->value = (element); \
>> +    _tmp->next = (list); \
>> +    list = _tmp; \
>> +} while (0)
> 
> Hmm - this seems like a frequently observed pattern - should it be
> something that we add independently and use throughout the tree as part
> of QAPI interactions in general?
> 
>> +
>> +BlockGraph *bdrv_get_block_graph(Error **errp)
>> +{
>> +    BlockGraph *graph = g_new0(BlockGraph, 1);
>> +    BlockDriverState *bs;
>> +    BdrvChild *child;
>> +    BlockGraphNode *node;
>> +    BlockGraphEdge *edge;
>> +    struct {
>> +        unsigned int flag;
>> +        BlockPermission num;
>> +    } permissions[] = {
>> +        { BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ },
> 
> Would it be worth directly reusing a QAPI enum for all such permissions
> in the existing code base, instead of having to map between an internal
> and a QAPI enum?
> 
>> +        { BLK_PERM_WRITE,           BLOCK_PERMISSION_WRITE },
>> +        { BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED },
>> +        { BLK_PERM_RESIZE,          BLOCK_PERMISSION_RESIZE },
>> +        { BLK_PERM_GRAPH_MOD,       BLOCK_PERMISSION_GRAPH_MOD },
>> +        { 0, 0 }
>> +    }, *p;
>> +
>> +    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
>> +        QLIST_FOREACH(child, &bs->parents, next_parent) {
>> +            if (!child->role->parent_is_bds) {
>> +                BlockGraphNodeList *el;
>> +                bool add = true;
> 
> Does your command need/want to expose internal nodes? (I argue that it
> probably should show internal nodes, even when normal 'query-block'
> doesn't, just because knowing about the impact of internal nodes can be
> important when tracking down permissions issues).

You could make it a flag, but in theory, implicit nodes should never be
visible through QMP.  (Though if you explicitly request them, that's
probably a different story.)

Max


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

Re: [Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Vladimir Sementsov-Ogievskiy]

17.08.2018 23:32, Eric Blake wrote:
> On 08/17/2018 01:04 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Add a new command, returning block nodes graph.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json  | 116 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>
>> +##
>> +# @BlockGraphEdge:
>> +#
>> +# Block Graph edge description for x-query-block-graph.
>> +#
>> +# @parent: parent id
>> +#
>> +# @child: child id
>> +#
>> +# @name: name of the relation (examples are 'file' and 'backing')
>
> Can this be made into a QAPI enum? (It would have ripple effects to 
> existing code, but might be a worthwhile cleanup).

I thought about it, but there no corresponding enum in Qemu too, so it's 
like driver name in query-block.. However, I can
create a function which will map &role pointer to corresponding qapi 
enum element, which will need to be updated when new
roles created. Not sure that it's better.

>
>> +#
>> +# @perm: granted permissions for the parent operating on the child
>> +#
>> +# @shared-perm: permissions that can still be granted to other users 
>> of the
>> +#               child while it is still attached this parent
>> +#
>> +# Since: 3.1
>> +##
>> +{ 'struct': 'BlockGraphEdge',
>> +  'data': { 'parent': 'uint64', 'child': 'uint64',
>> +            'name': 'str', 'perm': [ 'BlockPermission' ],
>> +            'shared-perm': [ 'BlockPermission' ] } }
>> +
>
>> +##
>> +# @x-query-block-graph:
>> +#
>> +# Get the block graph.
>> +#
>> +# Since: 3.1
>> +##
>> +{ 'command': 'x-query-block-graph', 'returns': 'BlockGraph' }
>
> Why is this command given an x- prefix?  What would it take to promote 
> it from experimental to fully supported?

Hmm, just a new command, used mostly for debugging. I don't have any 
special plans on improving it, I'm ok to drop x- prefix.

>
> Overall, the command looks quite useful; and the fact that you 
> produced some nice .dot graphs from it for visualization seems like it 
> is worth considering this as a permanent API addition. The question, 
> then, is if the interface is right, or if it needs any tweaks (like 
> using an enum instead of open-coded string for the relation between 
> parent and child), as a reason for keeping the x- prefix.

Yes, we should decide about enum.. If we want a qapi enum, how to mirror 
it in internal qemu structures better? Only by mapping function (ptr -> 
enum-element), or may be add field of EnumType entirely into the role 
structure?

>
>> +++ b/block.c
>> @@ -4003,6 +4003,86 @@ BlockDeviceInfoList 
>> *bdrv_named_nodes_list(Error **errp)
>>       return list;
>>   }
>>   +#define QAPI_LIST_ADD(list, element) do { \
>> +    typeof(list) _tmp = g_new(typeof(*(list)), 1); \
>> +    _tmp->value = (element); \
>> +    _tmp->next = (list); \
>> +    list = _tmp; \
>> +} while (0)
>
> Hmm - this seems like a frequently observed pattern - should it be 
> something that we add independently and use throughout the tree as 
> part of QAPI interactions in general?

Agree.

>
>> +
>> +BlockGraph *bdrv_get_block_graph(Error **errp)
>> +{
>> +    BlockGraph *graph = g_new0(BlockGraph, 1);
>> +    BlockDriverState *bs;
>> +    BdrvChild *child;
>> +    BlockGraphNode *node;
>> +    BlockGraphEdge *edge;
>> +    struct {
>> +        unsigned int flag;
>> +        BlockPermission num;
>> +    } permissions[] = {
>> +        { BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ },
>
> Would it be worth directly reusing a QAPI enum for all such 
> permissions in the existing code base, instead of having to map 
> between an internal and a QAPI enum?

Internal is not enum, they are flags.. How can we use enum instead? Or I 
don't understand what you mean..

>
>> +        { BLK_PERM_WRITE, BLOCK_PERMISSION_WRITE },
>> +        { BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED },
>> +        { BLK_PERM_RESIZE,          BLOCK_PERMISSION_RESIZE },
>> +        { BLK_PERM_GRAPH_MOD,       BLOCK_PERMISSION_GRAPH_MOD },
>> +        { 0, 0 }
>> +    }, *p;
>> +
>> +    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
>> +        QLIST_FOREACH(child, &bs->parents, next_parent) {
>> +            if (!child->role->parent_is_bds) {
>> +                BlockGraphNodeList *el;
>> +                bool add = true;
>
> Does your command need/want to expose internal nodes? (I argue that it 
> probably should show internal nodes, even when normal 'query-block' 
> doesn't, just because knowing about the impact of internal nodes can 
> be important when tracking down permissions issues).
>

Yes, I'm for viewing maximum number of nodes.. However my code goes 
through graph_bdrv_stats, query-named-block-nodes do the same.. Isn't it 
the whole set of nodes? All named... But, as I understand, now all nodes 
are named?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Vladimir Sementsov-Ogievskiy]

18.08.2018 00:03, Max Reitz wrote:
> On 2018-08-17 22:32, Eric Blake wrote:
>> On 08/17/2018 01:04 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Add a new command, returning block nodes graph.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>    qapi/block-core.json  | 116
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>> +##
>>> +# @BlockGraphEdge:
>>> +#
>>> +# Block Graph edge description for x-query-block-graph.
>>> +#
>>> +# @parent: parent id
>>> +#
>>> +# @child: child id
>>> +#
>>> +# @name: name of the relation (examples are 'file' and 'backing')
>> Can this be made into a QAPI enum? (It would have ripple effects to
>> existing code, but might be a worthwhile cleanup).
>>
>>> +#
>>> +# @perm: granted permissions for the parent operating on the child
>>> +#
>>> +# @shared-perm: permissions that can still be granted to other users
>>> of the
>>> +#               child while it is still attached this parent
>>> +#
>>> +# Since: 3.1
>>> +##
>>> +{ 'struct': 'BlockGraphEdge',
>>> +  'data': { 'parent': 'uint64', 'child': 'uint64',
>>> +            'name': 'str', 'perm': [ 'BlockPermission' ],
>>> +            'shared-perm': [ 'BlockPermission' ] } }
>>> +
>>> +##
>>> +# @x-query-block-graph:
>>> +#
>>> +# Get the block graph.
>>> +#
>>> +# Since: 3.1
>>> +##
>>> +{ 'command': 'x-query-block-graph', 'returns': 'BlockGraph' }
>> Why is this command given an x- prefix?  What would it take to promote
>> it from experimental to fully supported?
> This is just a very bad reasons, but I'll give it anyway: We really want
> such a command but still don't have one.  So I doubt this is exactly
> what we want. :-)
>
> A better reason is that we probably do not want a single command to
> return the whole block graph.  Do we want the command to just return
> info for a single node, including just the node names of the children?
> Do we want the command to include child info on request (but start from
> a user-specified root)?
>
> Also, the interface is...  Er, weird?  Honestly, I don't quite see why
> we would want it like this without x-.
>
> Why use newly generated IDs instead of node names?  Why are those RAM
> addresses?  That is just so fishy.
>
> Because parents can be non-nodes?  Well, I suppose you better not
> include them in the graph like this, then.
>
> I don't see why the query command we want would include non-BDSs at all.
>
> It may be useful for debugging, so, er, well, with an x-debug- prefix,
> OK.  But the question then is whether it's useful enough to warrant
> having a separate query command that isn't precisely the command we want
> anyway.


>
> The first question we probably have to ask is whether the query command
> needs to output parent information.  If not, querying would probably
> start at some named node and then you'd go down from there (either with
> many queries or through a single one).
>
> If so, well, then we can still output parent information, but I'd say
> that then is purely informational and we don't need to "generate" new
> IDs for them.  Just have either a node-name there or a user-readable
> description (like it was in v1; although it has to be noted that such a
> user-readable description is useless to a management layer), but these
> new IDs are really not something I like.
>
>> Overall, the command looks quite useful; and the fact that you produced
>> some nice .dot graphs from it for visualization seems like it is worth
>> considering this as a permanent API addition.  The question, then, is if
>> the interface is right, or if it needs any tweaks (like using an enum
>> instead of open-coded string for the relation between parent and child),
>> as a reason for keeping the x- prefix.
> You can use x-debug- even when you decide to keep a command.
>
> I see no reason why a command should hastily not get an x- prefix just
> because it may be useful enough.  If it really is and we really see the
> interface is good (which I really don't think it is), then we can always
> drop it later.
>
> Max
>
>>> +++ b/block.c
>>> @@ -4003,6 +4003,86 @@ BlockDeviceInfoList
>>> *bdrv_named_nodes_list(Error **errp)
>>>        return list;
>>>    }
>>>    +#define QAPI_LIST_ADD(list, element) do { \
>>> +    typeof(list) _tmp = g_new(typeof(*(list)), 1); \
>>> +    _tmp->value = (element); \
>>> +    _tmp->next = (list); \
>>> +    list = _tmp; \
>>> +} while (0)
>> Hmm - this seems like a frequently observed pattern - should it be
>> something that we add independently and use throughout the tree as part
>> of QAPI interactions in general?
>>
>>> +
>>> +BlockGraph *bdrv_get_block_graph(Error **errp)
>>> +{
>>> +    BlockGraph *graph = g_new0(BlockGraph, 1);
>>> +    BlockDriverState *bs;
>>> +    BdrvChild *child;
>>> +    BlockGraphNode *node;
>>> +    BlockGraphEdge *edge;
>>> +    struct {
>>> +        unsigned int flag;
>>> +        BlockPermission num;
>>> +    } permissions[] = {
>>> +        { BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ },
>> Would it be worth directly reusing a QAPI enum for all such permissions
>> in the existing code base, instead of having to map between an internal
>> and a QAPI enum?
>>
>>> +        { BLK_PERM_WRITE,           BLOCK_PERMISSION_WRITE },
>>> +        { BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED },
>>> +        { BLK_PERM_RESIZE,          BLOCK_PERMISSION_RESIZE },
>>> +        { BLK_PERM_GRAPH_MOD,       BLOCK_PERMISSION_GRAPH_MOD },
>>> +        { 0, 0 }
>>> +    }, *p;
>>> +
>>> +    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
>>> +        QLIST_FOREACH(child, &bs->parents, next_parent) {
>>> +            if (!child->role->parent_is_bds) {
>>> +                BlockGraphNodeList *el;
>>> +                bool add = true;
>> Does your command need/want to expose internal nodes? (I argue that it
>> probably should show internal nodes, even when normal 'query-block'
>> doesn't, just because knowing about the impact of internal nodes can be
>> important when tracking down permissions issues).
> You could make it a flag, but in theory, implicit nodes should never be
> visible through QMP.  (Though if you explicitly request them, that's
> probably a different story.)
>
> Max
>

My goal is to get graphviz representation of block graph with all the 
parents for debugging. And I'm absolutely ok to do it with x-debug-. 
Then we shouldn't care about enum for role type now. So, it the patch ok 
for you with x-debug- prefix?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Max Reitz]

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

On 2018-08-20 12:20, Vladimir Sementsov-Ogievskiy wrote:
> 18.08.2018 00:03, Max Reitz wrote:
>> On 2018-08-17 22:32, Eric Blake wrote:
>>> On 08/17/2018 01:04 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Add a new command, returning block nodes graph.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    qapi/block-core.json  | 116
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> +##
>>>> +# @BlockGraphEdge:
>>>> +#
>>>> +# Block Graph edge description for x-query-block-graph.
>>>> +#
>>>> +# @parent: parent id
>>>> +#
>>>> +# @child: child id
>>>> +#
>>>> +# @name: name of the relation (examples are 'file' and 'backing')
>>> Can this be made into a QAPI enum? (It would have ripple effects to
>>> existing code, but might be a worthwhile cleanup).
>>>
>>>> +#
>>>> +# @perm: granted permissions for the parent operating on the child
>>>> +#
>>>> +# @shared-perm: permissions that can still be granted to other users
>>>> of the
>>>> +#               child while it is still attached this parent
>>>> +#
>>>> +# Since: 3.1
>>>> +##
>>>> +{ 'struct': 'BlockGraphEdge',
>>>> +  'data': { 'parent': 'uint64', 'child': 'uint64',
>>>> +            'name': 'str', 'perm': [ 'BlockPermission' ],
>>>> +            'shared-perm': [ 'BlockPermission' ] } }
>>>> +
>>>> +##
>>>> +# @x-query-block-graph:
>>>> +#
>>>> +# Get the block graph.
>>>> +#
>>>> +# Since: 3.1
>>>> +##
>>>> +{ 'command': 'x-query-block-graph', 'returns': 'BlockGraph' }
>>> Why is this command given an x- prefix?  What would it take to promote
>>> it from experimental to fully supported?
>> This is just a very bad reasons, but I'll give it anyway: We really want
>> such a command but still don't have one.  So I doubt this is exactly
>> what we want. :-)
>>
>> A better reason is that we probably do not want a single command to
>> return the whole block graph.  Do we want the command to just return
>> info for a single node, including just the node names of the children?
>> Do we want the command to include child info on request (but start from
>> a user-specified root)?
>>
>> Also, the interface is...  Er, weird?  Honestly, I don't quite see why
>> we would want it like this without x-.
>>
>> Why use newly generated IDs instead of node names?  Why are those RAM
>> addresses?  That is just so fishy.
>>
>> Because parents can be non-nodes?  Well, I suppose you better not
>> include them in the graph like this, then.
>>
>> I don't see why the query command we want would include non-BDSs at all.
>>
>> It may be useful for debugging, so, er, well, with an x-debug- prefix,
>> OK.  But the question then is whether it's useful enough to warrant
>> having a separate query command that isn't precisely the command we want
>> anyway.
> 
> 
>>
>> The first question we probably have to ask is whether the query command
>> needs to output parent information.  If not, querying would probably
>> start at some named node and then you'd go down from there (either with
>> many queries or through a single one).
>>
>> If so, well, then we can still output parent information, but I'd say
>> that then is purely informational and we don't need to "generate" new
>> IDs for them.  Just have either a node-name there or a user-readable
>> description (like it was in v1; although it has to be noted that such a
>> user-readable description is useless to a management layer), but these
>> new IDs are really not something I like.
>>
>>> Overall, the command looks quite useful; and the fact that you produced
>>> some nice .dot graphs from it for visualization seems like it is worth
>>> considering this as a permanent API addition.  The question, then, is if
>>> the interface is right, or if it needs any tweaks (like using an enum
>>> instead of open-coded string for the relation between parent and child),
>>> as a reason for keeping the x- prefix.
>> You can use x-debug- even when you decide to keep a command.
>>
>> I see no reason why a command should hastily not get an x- prefix just
>> because it may be useful enough.  If it really is and we really see the
>> interface is good (which I really don't think it is), then we can always
>> drop it later.
>>
>> Max
>>
>>>> +++ b/block.c
>>>> @@ -4003,6 +4003,86 @@ BlockDeviceInfoList
>>>> *bdrv_named_nodes_list(Error **errp)
>>>>        return list;
>>>>    }
>>>>    +#define QAPI_LIST_ADD(list, element) do { \
>>>> +    typeof(list) _tmp = g_new(typeof(*(list)), 1); \
>>>> +    _tmp->value = (element); \
>>>> +    _tmp->next = (list); \
>>>> +    list = _tmp; \
>>>> +} while (0)
>>> Hmm - this seems like a frequently observed pattern - should it be
>>> something that we add independently and use throughout the tree as part
>>> of QAPI interactions in general?
>>>
>>>> +
>>>> +BlockGraph *bdrv_get_block_graph(Error **errp)
>>>> +{
>>>> +    BlockGraph *graph = g_new0(BlockGraph, 1);
>>>> +    BlockDriverState *bs;
>>>> +    BdrvChild *child;
>>>> +    BlockGraphNode *node;
>>>> +    BlockGraphEdge *edge;
>>>> +    struct {
>>>> +        unsigned int flag;
>>>> +        BlockPermission num;
>>>> +    } permissions[] = {
>>>> +        { BLK_PERM_CONSISTENT_READ,
>>>> BLOCK_PERMISSION_CONSISTENT_READ },
>>> Would it be worth directly reusing a QAPI enum for all such permissions
>>> in the existing code base, instead of having to map between an internal
>>> and a QAPI enum?
>>>
>>>> +        { BLK_PERM_WRITE,           BLOCK_PERMISSION_WRITE },
>>>> +        { BLK_PERM_WRITE_UNCHANGED,
>>>> BLOCK_PERMISSION_WRITE_UNCHANGED },
>>>> +        { BLK_PERM_RESIZE,          BLOCK_PERMISSION_RESIZE },
>>>> +        { BLK_PERM_GRAPH_MOD,       BLOCK_PERMISSION_GRAPH_MOD },
>>>> +        { 0, 0 }
>>>> +    }, *p;
>>>> +
>>>> +    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
>>>> +        QLIST_FOREACH(child, &bs->parents, next_parent) {
>>>> +            if (!child->role->parent_is_bds) {
>>>> +                BlockGraphNodeList *el;
>>>> +                bool add = true;
>>> Does your command need/want to expose internal nodes? (I argue that it
>>> probably should show internal nodes, even when normal 'query-block'
>>> doesn't, just because knowing about the impact of internal nodes can be
>>> important when tracking down permissions issues).
>> You could make it a flag, but in theory, implicit nodes should never be
>> visible through QMP.  (Though if you explicitly request them, that's
>> probably a different story.)
>>
>> Max
>>
> 
> My goal is to get graphviz representation of block graph with all the
> parents for debugging. And I'm absolutely ok to do it with x-debug-.
> Then we shouldn't care about enum for role type now. So, it the patch ok
> for you with x-debug- prefix?

Actually, no, because I'm not sure whether using points for the IDs is a
good idea.  That may defeat ASLR, and that would be a problem even with
x-debug-.

So I'd prefer using e.g. a hash map to map pointers to freshly generated
IDs (just incrementing integers).

In any case, I'll take a further look at the patch later, but another
thing that I've just seen is that using the opaque pointers to identify
a parent is something that doesn't look like it's guaranteed to work.

I suppose the alternative would be to start from all possible roots and
generate the graph from there (all possible roots being all
monitor-owned BDSs and all BlockBackends, I think).

Max


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

Re: [Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Vladimir Sementsov-Ogievskiy]

20.08.2018 16:44, Max Reitz wrote:
> On 2018-08-20 12:20, Vladimir Sementsov-Ogievskiy wrote:
>> 18.08.2018 00:03, Max Reitz wrote:
>>> On 2018-08-17 22:32, Eric Blake wrote:
>>>> On 08/17/2018 01:04 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Add a new command, returning block nodes graph.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>     qapi/block-core.json  | 116
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> +##
>>>>> +# @BlockGraphEdge:
>>>>> +#
>>>>> +# Block Graph edge description for x-query-block-graph.
>>>>> +#
>>>>> +# @parent: parent id
>>>>> +#
>>>>> +# @child: child id
>>>>> +#
>>>>> +# @name: name of the relation (examples are 'file' and 'backing')
>>>> Can this be made into a QAPI enum? (It would have ripple effects to
>>>> existing code, but might be a worthwhile cleanup).
>>>>
>>>>> +#
>>>>> +# @perm: granted permissions for the parent operating on the child
>>>>> +#
>>>>> +# @shared-perm: permissions that can still be granted to other users
>>>>> of the
>>>>> +#               child while it is still attached this parent
>>>>> +#
>>>>> +# Since: 3.1
>>>>> +##
>>>>> +{ 'struct': 'BlockGraphEdge',
>>>>> +  'data': { 'parent': 'uint64', 'child': 'uint64',
>>>>> +            'name': 'str', 'perm': [ 'BlockPermission' ],
>>>>> +            'shared-perm': [ 'BlockPermission' ] } }
>>>>> +
>>>>> +##
>>>>> +# @x-query-block-graph:
>>>>> +#
>>>>> +# Get the block graph.
>>>>> +#
>>>>> +# Since: 3.1
>>>>> +##
>>>>> +{ 'command': 'x-query-block-graph', 'returns': 'BlockGraph' }
>>>> Why is this command given an x- prefix?  What would it take to promote
>>>> it from experimental to fully supported?
>>> This is just a very bad reasons, but I'll give it anyway: We really want
>>> such a command but still don't have one.  So I doubt this is exactly
>>> what we want. :-)
>>>
>>> A better reason is that we probably do not want a single command to
>>> return the whole block graph.  Do we want the command to just return
>>> info for a single node, including just the node names of the children?
>>> Do we want the command to include child info on request (but start from
>>> a user-specified root)?
>>>
>>> Also, the interface is...  Er, weird?  Honestly, I don't quite see why
>>> we would want it like this without x-.
>>>
>>> Why use newly generated IDs instead of node names?  Why are those RAM
>>> addresses?  That is just so fishy.
>>>
>>> Because parents can be non-nodes?  Well, I suppose you better not
>>> include them in the graph like this, then.
>>>
>>> I don't see why the query command we want would include non-BDSs at all.
>>>
>>> It may be useful for debugging, so, er, well, with an x-debug- prefix,
>>> OK.  But the question then is whether it's useful enough to warrant
>>> having a separate query command that isn't precisely the command we want
>>> anyway.
>>
>>> The first question we probably have to ask is whether the query command
>>> needs to output parent information.  If not, querying would probably
>>> start at some named node and then you'd go down from there (either with
>>> many queries or through a single one).
>>>
>>> If so, well, then we can still output parent information, but I'd say
>>> that then is purely informational and we don't need to "generate" new
>>> IDs for them.  Just have either a node-name there or a user-readable
>>> description (like it was in v1; although it has to be noted that such a
>>> user-readable description is useless to a management layer), but these
>>> new IDs are really not something I like.
>>>
>>>> Overall, the command looks quite useful; and the fact that you produced
>>>> some nice .dot graphs from it for visualization seems like it is worth
>>>> considering this as a permanent API addition.  The question, then, is if
>>>> the interface is right, or if it needs any tweaks (like using an enum
>>>> instead of open-coded string for the relation between parent and child),
>>>> as a reason for keeping the x- prefix.
>>> You can use x-debug- even when you decide to keep a command.
>>>
>>> I see no reason why a command should hastily not get an x- prefix just
>>> because it may be useful enough.  If it really is and we really see the
>>> interface is good (which I really don't think it is), then we can always
>>> drop it later.
>>>
>>> Max
>>>
>>>>> +++ b/block.c
>>>>> @@ -4003,6 +4003,86 @@ BlockDeviceInfoList
>>>>> *bdrv_named_nodes_list(Error **errp)
>>>>>         return list;
>>>>>     }
>>>>>     +#define QAPI_LIST_ADD(list, element) do { \
>>>>> +    typeof(list) _tmp = g_new(typeof(*(list)), 1); \
>>>>> +    _tmp->value = (element); \
>>>>> +    _tmp->next = (list); \
>>>>> +    list = _tmp; \
>>>>> +} while (0)
>>>> Hmm - this seems like a frequently observed pattern - should it be
>>>> something that we add independently and use throughout the tree as part
>>>> of QAPI interactions in general?
>>>>
>>>>> +
>>>>> +BlockGraph *bdrv_get_block_graph(Error **errp)
>>>>> +{
>>>>> +    BlockGraph *graph = g_new0(BlockGraph, 1);
>>>>> +    BlockDriverState *bs;
>>>>> +    BdrvChild *child;
>>>>> +    BlockGraphNode *node;
>>>>> +    BlockGraphEdge *edge;
>>>>> +    struct {
>>>>> +        unsigned int flag;
>>>>> +        BlockPermission num;
>>>>> +    } permissions[] = {
>>>>> +        { BLK_PERM_CONSISTENT_READ,
>>>>> BLOCK_PERMISSION_CONSISTENT_READ },
>>>> Would it be worth directly reusing a QAPI enum for all such permissions
>>>> in the existing code base, instead of having to map between an internal
>>>> and a QAPI enum?
>>>>
>>>>> +        { BLK_PERM_WRITE,           BLOCK_PERMISSION_WRITE },
>>>>> +        { BLK_PERM_WRITE_UNCHANGED,
>>>>> BLOCK_PERMISSION_WRITE_UNCHANGED },
>>>>> +        { BLK_PERM_RESIZE,          BLOCK_PERMISSION_RESIZE },
>>>>> +        { BLK_PERM_GRAPH_MOD,       BLOCK_PERMISSION_GRAPH_MOD },
>>>>> +        { 0, 0 }
>>>>> +    }, *p;
>>>>> +
>>>>> +    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
>>>>> +        QLIST_FOREACH(child, &bs->parents, next_parent) {
>>>>> +            if (!child->role->parent_is_bds) {
>>>>> +                BlockGraphNodeList *el;
>>>>> +                bool add = true;
>>>> Does your command need/want to expose internal nodes? (I argue that it
>>>> probably should show internal nodes, even when normal 'query-block'
>>>> doesn't, just because knowing about the impact of internal nodes can be
>>>> important when tracking down permissions issues).
>>> You could make it a flag, but in theory, implicit nodes should never be
>>> visible through QMP.  (Though if you explicitly request them, that's
>>> probably a different story.)
>>>
>>> Max
>>>
>> My goal is to get graphviz representation of block graph with all the
>> parents for debugging. And I'm absolutely ok to do it with x-debug-.
>> Then we shouldn't care about enum for role type now. So, it the patch ok
>> for you with x-debug- prefix?
> Actually, no, because I'm not sure whether using points for the IDs is a
> good idea.  That may defeat ASLR, and that would be a problem even with
> x-debug-.

Good point, agree.

>
> So I'd prefer using e.g. a hash map to map pointers to freshly generated
> IDs (just incrementing integers).
>
> In any case, I'll take a further look at the patch later, but another
> thing that I've just seen is that using the opaque pointers to identify
> a parent is something that doesn't look like it's guaranteed to work.

Hm, isn't it a bug? Can you point to an example?

>
> I suppose the alternative would be to start from all possible roots and
> generate the graph from there (all possible roots being all
> monitor-owned BDSs and all BlockBackends, I think).
>
> Max
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Max Reitz]

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

On 2018-08-20 17:13, Vladimir Sementsov-Ogievskiy wrote:
> 20.08.2018 16:44, Max Reitz wrote:
>> On 2018-08-20 12:20, Vladimir Sementsov-Ogievskiy wrote:

[...]

>>> My goal is to get graphviz representation of block graph with all the
>>> parents for debugging. And I'm absolutely ok to do it with x-debug-.
>>> Then we shouldn't care about enum for role type now. So, it the patch ok
>>> for you with x-debug- prefix?
>> Actually, no, because I'm not sure whether using points for the IDs is a
>> good idea.  That may defeat ASLR, and that would be a problem even with
>> x-debug-.
> 
> Good point, agree.
> 
>>
>> So I'd prefer using e.g. a hash map to map pointers to freshly generated
>> IDs (just incrementing integers).

(By the way, that would also improve the speed of checking whether a
certain node needs to be added to the @nodes list still.)

>> In any case, I'll take a further look at the patch later, but another
>> thing that I've just seen is that using the opaque pointers to identify
>> a parent is something that doesn't look like it's guaranteed to work.
> 
> Hm, isn't it a bug? Can you point to an example?

Ah, no, it's OK.  Well, kind of.  It's OK in the sense that it is unique
when set.  I didn't notice that you only use it for the non-node
parents, sorry.

Still, it probably would be better to just use the BdrvChild object, as
that should be unique as well, and it is obviously non-NULL.
(BdrvChild.opaque may be NULL, even though it isn't in practice.)

Max


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

Re: [Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Vladimir Sementsov-Ogievskiy]

20.08.2018 19:35, Max Reitz wrote:
> On 2018-08-20 17:13, Vladimir Sementsov-Ogievskiy wrote:
>> 20.08.2018 16:44, Max Reitz wrote:
>>> On 2018-08-20 12:20, Vladimir Sementsov-Ogievskiy wrote:
> [...]
>
>>>> My goal is to get graphviz representation of block graph with all the
>>>> parents for debugging. And I'm absolutely ok to do it with x-debug-.
>>>> Then we shouldn't care about enum for role type now. So, it the patch ok
>>>> for you with x-debug- prefix?
>>> Actually, no, because I'm not sure whether using points for the IDs is a
>>> good idea.  That may defeat ASLR, and that would be a problem even with
>>> x-debug-.
>> Good point, agree.
>>
>>> So I'd prefer using e.g. a hash map to map pointers to freshly generated
>>> IDs (just incrementing integers).
> (By the way, that would also improve the speed of checking whether a
> certain node needs to be added to the @nodes list still.)
>
>>> In any case, I'll take a further look at the patch later, but another
>>> thing that I've just seen is that using the opaque pointers to identify
>>> a parent is something that doesn't look like it's guaranteed to work.
>> Hm, isn't it a bug? Can you point to an example?
> Ah, no, it's OK.  Well, kind of.  It's OK in the sense that it is unique
> when set.  I didn't notice that you only use it for the non-node
> parents, sorry.

hm, no, I use opaque for all. So, it can be zero? In what case? In this 
case, I cant get its parent?

>
> Still, it probably would be better to just use the BdrvChild object, as
> that should be unique as well, and it is obviously non-NULL.
> (BdrvChild.opaque may be NULL, even though it isn't in practice.)

but BdrvChild corresponds to edge in a graph, not to the node. I need 
identificators for nodes..

>
> Max
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Max Reitz]

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

On 2018-08-20 19:04, Vladimir Sementsov-Ogievskiy wrote:
> 20.08.2018 19:35, Max Reitz wrote:
>> On 2018-08-20 17:13, Vladimir Sementsov-Ogievskiy wrote:
>>> 20.08.2018 16:44, Max Reitz wrote:
>>>> On 2018-08-20 12:20, Vladimir Sementsov-Ogievskiy wrote:
>> [...]
>>
>>>>> My goal is to get graphviz representation of block graph with all the
>>>>> parents for debugging. And I'm absolutely ok to do it with x-debug-.
>>>>> Then we shouldn't care about enum for role type now. So, it the
>>>>> patch ok
>>>>> for you with x-debug- prefix?
>>>> Actually, no, because I'm not sure whether using points for the IDs
>>>> is a
>>>> good idea.  That may defeat ASLR, and that would be a problem even with
>>>> x-debug-.
>>> Good point, agree.
>>>
>>>> So I'd prefer using e.g. a hash map to map pointers to freshly
>>>> generated
>>>> IDs (just incrementing integers).
>> (By the way, that would also improve the speed of checking whether a
>> certain node needs to be added to the @nodes list still.)
>>
>>>> In any case, I'll take a further look at the patch later, but another
>>>> thing that I've just seen is that using the opaque pointers to identify
>>>> a parent is something that doesn't look like it's guaranteed to work.
>>> Hm, isn't it a bug? Can you point to an example?
>> Ah, no, it's OK.  Well, kind of.  It's OK in the sense that it is unique
>> when set.  I didn't notice that you only use it for the non-node
>> parents, sorry.
> 
> hm, no, I use opaque for all. So, it can be zero? In what case? In this
> case, I cant get its parent?

As far as I can see, you only use it for BLOCK_GRAPH_NODE_TYPE_OTHER,
and you only do it when the parent is indeed not a BDS (because BDS are
always children of some sort, so they'll be automatically accounted for
anyway).

Ah, but you set edge->parent = (uint64_t)child->opaque.  I see, because
you assume it's always going to point to the BDS.

Yeah, generally, you can't get the parent.  That's the whole point.  I
suppose in practice your code works, but that's not how it's supposed to
be.  You generally cannot go from child to parent, only through the
interface defined through BdrvChildRole (which uses the opaque pointer).

As I said, a safe way to do this would be to enumerate all possible
block graph roots; those being (as far as I know) all BlockBackends and
all monitor-owned BDSs (see bdrv_close_all(), which handles exactly
those two cases).  Then you can walk down from the roots through the trees.

>> Still, it probably would be better to just use the BdrvChild object, as
>> that should be unique as well, and it is obviously non-NULL.
>> (BdrvChild.opaque may be NULL, even though it isn't in practice.)
> 
> but BdrvChild corresponds to edge in a graph, not to the node. I need
> identificators for nodes..

But an edge identifies two nodes.  All edges have at least one BDS on
one end.

Since you can identify all BDS through the BDS itself, if you have a
non-BDS node, you can use the edge to identify it (because the other end
of the edge is going to be a BDS which you can identify by itself).

Or, well, if you take my suggestion and walk down the trees starting at
the roots, you'll see that the only non-BDS nodes in the block graph are
BlockBackends.  And you can just use their addresses to identify them
(just internally, of course; externally you'll still want to generate a
new ID).

Max


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

Re: [Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Vladimir Sementsov-Ogievskiy]

20.08.2018 20:13, Max Reitz wrote:
> On 2018-08-20 19:04, Vladimir Sementsov-Ogievskiy wrote:
>> 20.08.2018 19:35, Max Reitz wrote:
>>> On 2018-08-20 17:13, Vladimir Sementsov-Ogievskiy wrote:
>>>> 20.08.2018 16:44, Max Reitz wrote:
>>>>> On 2018-08-20 12:20, Vladimir Sementsov-Ogievskiy wrote:
>>> [...]
>>>
>>>>>> My goal is to get graphviz representation of block graph with all the
>>>>>> parents for debugging. And I'm absolutely ok to do it with x-debug-.
>>>>>> Then we shouldn't care about enum for role type now. So, it the
>>>>>> patch ok
>>>>>> for you with x-debug- prefix?
>>>>> Actually, no, because I'm not sure whether using points for the IDs
>>>>> is a
>>>>> good idea.  That may defeat ASLR, and that would be a problem even with
>>>>> x-debug-.
>>>> Good point, agree.
>>>>
>>>>> So I'd prefer using e.g. a hash map to map pointers to freshly
>>>>> generated
>>>>> IDs (just incrementing integers).
>>> (By the way, that would also improve the speed of checking whether a
>>> certain node needs to be added to the @nodes list still.)
>>>
>>>>> In any case, I'll take a further look at the patch later, but another
>>>>> thing that I've just seen is that using the opaque pointers to identify
>>>>> a parent is something that doesn't look like it's guaranteed to work.
>>>> Hm, isn't it a bug? Can you point to an example?
>>> Ah, no, it's OK.  Well, kind of.  It's OK in the sense that it is unique
>>> when set.  I didn't notice that you only use it for the non-node
>>> parents, sorry.
>> hm, no, I use opaque for all. So, it can be zero? In what case? In this
>> case, I cant get its parent?
> As far as I can see, you only use it for BLOCK_GRAPH_NODE_TYPE_OTHER,
> and you only do it when the parent is indeed not a BDS (because BDS are
> always children of some sort, so they'll be automatically accounted for
> anyway).
>
> Ah, but you set edge->parent = (uint64_t)child->opaque.  I see, because
> you assume it's always going to point to the BDS.

... or to some non-bds parent

>
> Yeah, generally, you can't get the parent.  That's the whole point.  I
> suppose in practice your code works, but that's not how it's supposed to
> be.  You generally cannot go from child to parent, only through the
> interface defined through BdrvChildRole (which uses the opaque pointer).
>
> As I said, a safe way to do this would be to enumerate all possible
> block graph roots; those being (as far as I know) all BlockBackends and
> all monitor-owned BDSs (see bdrv_close_all(), which handles exactly
> those two cases).  Then you can walk down from the roots through the trees.

hmm, what about block jobs here?

>
>>> Still, it probably would be better to just use the BdrvChild object, as
>>> that should be unique as well, and it is obviously non-NULL.
>>> (BdrvChild.opaque may be NULL, even though it isn't in practice.)
>> but BdrvChild corresponds to edge in a graph, not to the node. I need
>> identificators for nodes..
> But an edge identifies two nodes.  All edges have at least one BDS on
> one end.
>
> Since you can identify all BDS through the BDS itself, if you have a
> non-BDS node, you can use the edge to identify it (because the other end
> of the edge is going to be a BDS which you can identify by itself).

But non-bds node may have several children, therefore several 
BdrvChild's with different pointers.. And only opque point should be the 
same for them.

>
> Or, well, if you take my suggestion and walk down the trees starting at
> the roots, you'll see that the only non-BDS nodes in the block graph are
> BlockBackends.  And you can just use their addresses to identify them
> (just internally, of course; externally you'll still want to generate a
> new ID).

As I understand, I'll not see block jobs in this way to be non-bds nodes..

>
> Max
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Vladimir Sementsov-Ogievskiy]

20.08.2018 16:44, Max Reitz wrote:
> On 2018-08-20 12:20, Vladimir Sementsov-Ogievskiy wrote:
>> 18.08.2018 00:03, Max Reitz wrote:
>>> On 2018-08-17 22:32, Eric Blake wrote:
>>>> On 08/17/2018 01:04 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Add a new command, returning block nodes graph.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>     qapi/block-core.json  | 116
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> +##
>>>>> +# @BlockGraphEdge:
>>>>> +#
>>>>> +# Block Graph edge description for x-query-block-graph.
>>>>> +#
>>>>> +# @parent: parent id
>>>>> +#
>>>>> +# @child: child id
>>>>> +#
>>>>> +# @name: name of the relation (examples are 'file' and 'backing')
>>>> Can this be made into a QAPI enum? (It would have ripple effects to
>>>> existing code, but might be a worthwhile cleanup).
>>>>
>>>>> +#
>>>>> +# @perm: granted permissions for the parent operating on the child
>>>>> +#
>>>>> +# @shared-perm: permissions that can still be granted to other users
>>>>> of the
>>>>> +#               child while it is still attached this parent
>>>>> +#
>>>>> +# Since: 3.1
>>>>> +##
>>>>> +{ 'struct': 'BlockGraphEdge',
>>>>> +  'data': { 'parent': 'uint64', 'child': 'uint64',
>>>>> +            'name': 'str', 'perm': [ 'BlockPermission' ],
>>>>> +            'shared-perm': [ 'BlockPermission' ] } }
>>>>> +
>>>>> +##
>>>>> +# @x-query-block-graph:
>>>>> +#
>>>>> +# Get the block graph.
>>>>> +#
>>>>> +# Since: 3.1
>>>>> +##
>>>>> +{ 'command': 'x-query-block-graph', 'returns': 'BlockGraph' }
>>>> Why is this command given an x- prefix?  What would it take to promote
>>>> it from experimental to fully supported?
>>> This is just a very bad reasons, but I'll give it anyway: We really want
>>> such a command but still don't have one.  So I doubt this is exactly
>>> what we want. :-)
>>>
>>> A better reason is that we probably do not want a single command to
>>> return the whole block graph.  Do we want the command to just return
>>> info for a single node, including just the node names of the children?
>>> Do we want the command to include child info on request (but start from
>>> a user-specified root)?
>>>
>>> Also, the interface is...  Er, weird?  Honestly, I don't quite see why
>>> we would want it like this without x-.
>>>
>>> Why use newly generated IDs instead of node names?  Why are those RAM
>>> addresses?  That is just so fishy.
>>>
>>> Because parents can be non-nodes?  Well, I suppose you better not
>>> include them in the graph like this, then.
>>>
>>> I don't see why the query command we want would include non-BDSs at all.
>>>
>>> It may be useful for debugging, so, er, well, with an x-debug- prefix,
>>> OK.  But the question then is whether it's useful enough to warrant
>>> having a separate query command that isn't precisely the command we want
>>> anyway.
>>
>>> The first question we probably have to ask is whether the query command
>>> needs to output parent information.  If not, querying would probably
>>> start at some named node and then you'd go down from there (either with
>>> many queries or through a single one).
>>>
>>> If so, well, then we can still output parent information, but I'd say
>>> that then is purely informational and we don't need to "generate" new
>>> IDs for them.  Just have either a node-name there or a user-readable
>>> description (like it was in v1; although it has to be noted that such a
>>> user-readable description is useless to a management layer), but these
>>> new IDs are really not something I like.
>>>
>>>> Overall, the command looks quite useful; and the fact that you produced
>>>> some nice .dot graphs from it for visualization seems like it is worth
>>>> considering this as a permanent API addition.  The question, then, is if
>>>> the interface is right, or if it needs any tweaks (like using an enum
>>>> instead of open-coded string for the relation between parent and child),
>>>> as a reason for keeping the x- prefix.
>>> You can use x-debug- even when you decide to keep a command.
>>>
>>> I see no reason why a command should hastily not get an x- prefix just
>>> because it may be useful enough.  If it really is and we really see the
>>> interface is good (which I really don't think it is), then we can always
>>> drop it later.
>>>
>>> Max
>>>
>>>>> +++ b/block.c
>>>>> @@ -4003,6 +4003,86 @@ BlockDeviceInfoList
>>>>> *bdrv_named_nodes_list(Error **errp)
>>>>>         return list;
>>>>>     }
>>>>>     +#define QAPI_LIST_ADD(list, element) do { \
>>>>> +    typeof(list) _tmp = g_new(typeof(*(list)), 1); \
>>>>> +    _tmp->value = (element); \
>>>>> +    _tmp->next = (list); \
>>>>> +    list = _tmp; \
>>>>> +} while (0)
>>>> Hmm - this seems like a frequently observed pattern - should it be
>>>> something that we add independently and use throughout the tree as part
>>>> of QAPI interactions in general?
>>>>
>>>>> +
>>>>> +BlockGraph *bdrv_get_block_graph(Error **errp)
>>>>> +{
>>>>> +    BlockGraph *graph = g_new0(BlockGraph, 1);
>>>>> +    BlockDriverState *bs;
>>>>> +    BdrvChild *child;
>>>>> +    BlockGraphNode *node;
>>>>> +    BlockGraphEdge *edge;
>>>>> +    struct {
>>>>> +        unsigned int flag;
>>>>> +        BlockPermission num;
>>>>> +    } permissions[] = {
>>>>> +        { BLK_PERM_CONSISTENT_READ,
>>>>> BLOCK_PERMISSION_CONSISTENT_READ },
>>>> Would it be worth directly reusing a QAPI enum for all such permissions
>>>> in the existing code base, instead of having to map between an internal
>>>> and a QAPI enum?
>>>>
>>>>> +        { BLK_PERM_WRITE,           BLOCK_PERMISSION_WRITE },
>>>>> +        { BLK_PERM_WRITE_UNCHANGED,
>>>>> BLOCK_PERMISSION_WRITE_UNCHANGED },
>>>>> +        { BLK_PERM_RESIZE,          BLOCK_PERMISSION_RESIZE },
>>>>> +        { BLK_PERM_GRAPH_MOD,       BLOCK_PERMISSION_GRAPH_MOD },
>>>>> +        { 0, 0 }
>>>>> +    }, *p;
>>>>> +
>>>>> +    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
>>>>> +        QLIST_FOREACH(child, &bs->parents, next_parent) {
>>>>> +            if (!child->role->parent_is_bds) {
>>>>> +                BlockGraphNodeList *el;
>>>>> +                bool add = true;
>>>> Does your command need/want to expose internal nodes? (I argue that it
>>>> probably should show internal nodes, even when normal 'query-block'
>>>> doesn't, just because knowing about the impact of internal nodes can be
>>>> important when tracking down permissions issues).
>>> You could make it a flag, but in theory, implicit nodes should never be
>>> visible through QMP.  (Though if you explicitly request them, that's
>>> probably a different story.)
>>>
>>> Max
>>>
>> My goal is to get graphviz representation of block graph with all the
>> parents for debugging. And I'm absolutely ok to do it with x-debug-.
>> Then we shouldn't care about enum for role type now. So, it the patch ok
>> for you with x-debug- prefix?
> Actually, no, because I'm not sure whether using points for the IDs is a
> good idea.  That may defeat ASLR, and that would be a problem even with
> x-debug-.

Hmm ASLR, what about trace points? Do they violate it? We can enable 
trace-points on running vm and they'll show a lot of pointers...

>
> So I'd prefer using e.g. a hash map to map pointers to freshly generated
> IDs (just incrementing integers).
>
> In any case, I'll take a further look at the patch later, but another
> thing that I've just seen is that using the opaque pointers to identify
> a parent is something that doesn't look like it's guaranteed to work.
>
> I suppose the alternative would be to start from all possible roots and
> generate the graph from there (all possible roots being all
> monitor-owned BDSs and all BlockBackends, I think).
>
> Max
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Max Reitz]

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

On 2018-08-20 20:38, Vladimir Sementsov-Ogievskiy wrote:
> 20.08.2018 16:44, Max Reitz wrote:
>> On 2018-08-20 12:20, Vladimir Sementsov-Ogievskiy wrote:
>>> 18.08.2018 00:03, Max Reitz wrote:
>>>> On 2018-08-17 22:32, Eric Blake wrote:
>>>>> On 08/17/2018 01:04 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Add a new command, returning block nodes graph.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>>>>> <vsementsov@virtuozzo.com>
>>>>>> ---
>>>>>>     qapi/block-core.json  | 116
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> +##
>>>>>> +# @BlockGraphEdge:
>>>>>> +#
>>>>>> +# Block Graph edge description for x-query-block-graph.
>>>>>> +#
>>>>>> +# @parent: parent id
>>>>>> +#
>>>>>> +# @child: child id
>>>>>> +#
>>>>>> +# @name: name of the relation (examples are 'file' and 'backing')
>>>>> Can this be made into a QAPI enum? (It would have ripple effects to
>>>>> existing code, but might be a worthwhile cleanup).
>>>>>
>>>>>> +#
>>>>>> +# @perm: granted permissions for the parent operating on the child
>>>>>> +#
>>>>>> +# @shared-perm: permissions that can still be granted to other users
>>>>>> of the
>>>>>> +#               child while it is still attached this parent
>>>>>> +#
>>>>>> +# Since: 3.1
>>>>>> +##
>>>>>> +{ 'struct': 'BlockGraphEdge',
>>>>>> +  'data': { 'parent': 'uint64', 'child': 'uint64',
>>>>>> +            'name': 'str', 'perm': [ 'BlockPermission' ],
>>>>>> +            'shared-perm': [ 'BlockPermission' ] } }
>>>>>> +
>>>>>> +##
>>>>>> +# @x-query-block-graph:
>>>>>> +#
>>>>>> +# Get the block graph.
>>>>>> +#
>>>>>> +# Since: 3.1
>>>>>> +##
>>>>>> +{ 'command': 'x-query-block-graph', 'returns': 'BlockGraph' }
>>>>> Why is this command given an x- prefix?  What would it take to promote
>>>>> it from experimental to fully supported?
>>>> This is just a very bad reasons, but I'll give it anyway: We really
>>>> want
>>>> such a command but still don't have one.  So I doubt this is exactly
>>>> what we want. :-)
>>>>
>>>> A better reason is that we probably do not want a single command to
>>>> return the whole block graph.  Do we want the command to just return
>>>> info for a single node, including just the node names of the children?
>>>> Do we want the command to include child info on request (but start from
>>>> a user-specified root)?
>>>>
>>>> Also, the interface is...  Er, weird?  Honestly, I don't quite see why
>>>> we would want it like this without x-.
>>>>
>>>> Why use newly generated IDs instead of node names?  Why are those RAM
>>>> addresses?  That is just so fishy.
>>>>
>>>> Because parents can be non-nodes?  Well, I suppose you better not
>>>> include them in the graph like this, then.
>>>>
>>>> I don't see why the query command we want would include non-BDSs at
>>>> all.
>>>>
>>>> It may be useful for debugging, so, er, well, with an x-debug- prefix,
>>>> OK.  But the question then is whether it's useful enough to warrant
>>>> having a separate query command that isn't precisely the command we
>>>> want
>>>> anyway.
>>>
>>>> The first question we probably have to ask is whether the query command
>>>> needs to output parent information.  If not, querying would probably
>>>> start at some named node and then you'd go down from there (either with
>>>> many queries or through a single one).
>>>>
>>>> If so, well, then we can still output parent information, but I'd say
>>>> that then is purely informational and we don't need to "generate" new
>>>> IDs for them.  Just have either a node-name there or a user-readable
>>>> description (like it was in v1; although it has to be noted that such a
>>>> user-readable description is useless to a management layer), but these
>>>> new IDs are really not something I like.
>>>>
>>>>> Overall, the command looks quite useful; and the fact that you
>>>>> produced
>>>>> some nice .dot graphs from it for visualization seems like it is worth
>>>>> considering this as a permanent API addition.  The question, then,
>>>>> is if
>>>>> the interface is right, or if it needs any tweaks (like using an enum
>>>>> instead of open-coded string for the relation between parent and
>>>>> child),
>>>>> as a reason for keeping the x- prefix.
>>>> You can use x-debug- even when you decide to keep a command.
>>>>
>>>> I see no reason why a command should hastily not get an x- prefix just
>>>> because it may be useful enough.  If it really is and we really see the
>>>> interface is good (which I really don't think it is), then we can
>>>> always
>>>> drop it later.
>>>>
>>>> Max
>>>>
>>>>>> +++ b/block.c
>>>>>> @@ -4003,6 +4003,86 @@ BlockDeviceInfoList
>>>>>> *bdrv_named_nodes_list(Error **errp)
>>>>>>         return list;
>>>>>>     }
>>>>>>     +#define QAPI_LIST_ADD(list, element) do { \
>>>>>> +    typeof(list) _tmp = g_new(typeof(*(list)), 1); \
>>>>>> +    _tmp->value = (element); \
>>>>>> +    _tmp->next = (list); \
>>>>>> +    list = _tmp; \
>>>>>> +} while (0)
>>>>> Hmm - this seems like a frequently observed pattern - should it be
>>>>> something that we add independently and use throughout the tree as
>>>>> part
>>>>> of QAPI interactions in general?
>>>>>
>>>>>> +
>>>>>> +BlockGraph *bdrv_get_block_graph(Error **errp)
>>>>>> +{
>>>>>> +    BlockGraph *graph = g_new0(BlockGraph, 1);
>>>>>> +    BlockDriverState *bs;
>>>>>> +    BdrvChild *child;
>>>>>> +    BlockGraphNode *node;
>>>>>> +    BlockGraphEdge *edge;
>>>>>> +    struct {
>>>>>> +        unsigned int flag;
>>>>>> +        BlockPermission num;
>>>>>> +    } permissions[] = {
>>>>>> +        { BLK_PERM_CONSISTENT_READ,
>>>>>> BLOCK_PERMISSION_CONSISTENT_READ },
>>>>> Would it be worth directly reusing a QAPI enum for all such
>>>>> permissions
>>>>> in the existing code base, instead of having to map between an
>>>>> internal
>>>>> and a QAPI enum?
>>>>>
>>>>>> +        { BLK_PERM_WRITE,           BLOCK_PERMISSION_WRITE },
>>>>>> +        { BLK_PERM_WRITE_UNCHANGED,
>>>>>> BLOCK_PERMISSION_WRITE_UNCHANGED },
>>>>>> +        { BLK_PERM_RESIZE,          BLOCK_PERMISSION_RESIZE },
>>>>>> +        { BLK_PERM_GRAPH_MOD,       BLOCK_PERMISSION_GRAPH_MOD },
>>>>>> +        { 0, 0 }
>>>>>> +    }, *p;
>>>>>> +
>>>>>> +    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
>>>>>> +        QLIST_FOREACH(child, &bs->parents, next_parent) {
>>>>>> +            if (!child->role->parent_is_bds) {
>>>>>> +                BlockGraphNodeList *el;
>>>>>> +                bool add = true;
>>>>> Does your command need/want to expose internal nodes? (I argue that it
>>>>> probably should show internal nodes, even when normal 'query-block'
>>>>> doesn't, just because knowing about the impact of internal nodes
>>>>> can be
>>>>> important when tracking down permissions issues).
>>>> You could make it a flag, but in theory, implicit nodes should never be
>>>> visible through QMP.  (Though if you explicitly request them, that's
>>>> probably a different story.)
>>>>
>>>> Max
>>>>
>>> My goal is to get graphviz representation of block graph with all the
>>> parents for debugging. And I'm absolutely ok to do it with x-debug-.
>>> Then we shouldn't care about enum for role type now. So, it the patch ok
>>> for you with x-debug- prefix?
>> Actually, no, because I'm not sure whether using points for the IDs is a
>> good idea.  That may defeat ASLR, and that would be a problem even with
>> x-debug-.
> 
> Hmm ASLR, what about trace points? Do they violate it? We can enable
> trace-points on running vm and they'll show a lot of pointers...

Good question, but at least you can disable it during configure.  If
query-block-graph is disabled by default and needs to be enabled
explicitly, I guess that'd be OK.

Another benefit of using a hash map though would be that you don't need
to loop through the array of existing nodes to find out which you have
added already -- a lookup in the hash map would be enough.

Max

>> So I'd prefer using e.g. a hash map to map pointers to freshly generated
>> IDs (just incrementing integers).
>>
>> In any case, I'll take a further look at the patch later, but another
>> thing that I've just seen is that using the opaque pointers to identify
>> a parent is something that doesn't look like it's guaranteed to work.
>>
>> I suppose the alternative would be to start from all possible roots and
>> generate the graph from there (all possible roots being all
>> monitor-owned BDSs and all BlockBackends, I think).
>>
>> Max
>>
> 
> 



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

Re: [Qemu-devel] [PATCH v2 1/3] qapi: add x-query-block-graph

[Vladimir Sementsov-Ogievskiy]

20.08.2018 20:35, Vladimir Sementsov-Ogievskiy wrote:
> 20.08.2018 20:13, Max Reitz wrote:
>> On 2018-08-20 19:04, Vladimir Sementsov-Ogievskiy wrote:
>>> 20.08.2018 19:35, Max Reitz wrote:
>>>> On 2018-08-20 17:13, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 20.08.2018 16:44, Max Reitz wrote:
>>>>>> On 2018-08-20 12:20, Vladimir Sementsov-Ogievskiy wrote:
>>>> [...]
>>>>
>>>>>>> My goal is to get graphviz representation of block graph with 
>>>>>>> all the
>>>>>>> parents for debugging. And I'm absolutely ok to do it with 
>>>>>>> x-debug-.
>>>>>>> Then we shouldn't care about enum for role type now. So, it the
>>>>>>> patch ok
>>>>>>> for you with x-debug- prefix?
>>>>>> Actually, no, because I'm not sure whether using points for the IDs
>>>>>> is a
>>>>>> good idea.  That may defeat ASLR, and that would be a problem 
>>>>>> even with
>>>>>> x-debug-.
>>>>> Good point, agree.
>>>>>
>>>>>> So I'd prefer using e.g. a hash map to map pointers to freshly
>>>>>> generated
>>>>>> IDs (just incrementing integers).
>>>> (By the way, that would also improve the speed of checking whether a
>>>> certain node needs to be added to the @nodes list still.)
>>>>
>>>>>> In any case, I'll take a further look at the patch later, but 
>>>>>> another
>>>>>> thing that I've just seen is that using the opaque pointers to 
>>>>>> identify
>>>>>> a parent is something that doesn't look like it's guaranteed to 
>>>>>> work.
>>>>> Hm, isn't it a bug? Can you point to an example?
>>>> Ah, no, it's OK.  Well, kind of.  It's OK in the sense that it is 
>>>> unique
>>>> when set.  I didn't notice that you only use it for the non-node
>>>> parents, sorry.
>>> hm, no, I use opaque for all. So, it can be zero? In what case? In this
>>> case, I cant get its parent?
>> As far as I can see, you only use it for BLOCK_GRAPH_NODE_TYPE_OTHER,
>> and you only do it when the parent is indeed not a BDS (because BDS are
>> always children of some sort, so they'll be automatically accounted for
>> anyway).
>>
>> Ah, but you set edge->parent = (uint64_t)child->opaque.  I see, because
>> you assume it's always going to point to the BDS.
>
> ... or to some non-bds parent
>
>>
>> Yeah, generally, you can't get the parent.  That's the whole point.  I
>> suppose in practice your code works, but that's not how it's supposed to
>> be.  You generally cannot go from child to parent, only through the
>> interface defined through BdrvChildRole (which uses the opaque pointer).
>>
>> As I said, a safe way to do this would be to enumerate all possible
>> block graph roots; those being (as far as I know) all BlockBackends and
>> all monitor-owned BDSs (see bdrv_close_all(), which handles exactly
>> those two cases).  Then you can walk down from the roots through the 
>> trees.
>
> hmm, what about block jobs here?
>
>>
>>>> Still, it probably would be better to just use the BdrvChild 
>>>> object, as
>>>> that should be unique as well, and it is obviously non-NULL.
>>>> (BdrvChild.opaque may be NULL, even though it isn't in practice.)
>>> but BdrvChild corresponds to edge in a graph, not to the node. I need
>>> identificators for nodes..
>> But an edge identifies two nodes.  All edges have at least one BDS on
>> one end.
>>
>> Since you can identify all BDS through the BDS itself, if you have a
>> non-BDS node, you can use the edge to identify it (because the other end
>> of the edge is going to be a BDS which you can identify by itself).
>
> But non-bds node may have several children, therefore several 
> BdrvChild's with different pointers.. And only opque point should be 
> the same for them.
>
>>
>> Or, well, if you take my suggestion and walk down the trees starting at
>> the roots, you'll see that the only non-BDS nodes in the block graph are
>> BlockBackends.  And you can just use their addresses to identify them
>> (just internally, of course; externally you'll still want to generate a
>> new ID).
>
> As I understand, I'll not see block jobs in this way to be non-bds 
> nodes..

and no problem to loop through block jobs. hmm.

In it is interesting to show blks and jobs even if the don't have bds 
children, for the general picture.

So, I'll try this way.

>
>>
>> Max
>>
>
>


-- 
Best regards,
Vladimir