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

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

[Vladimir Sementsov-Ogievskiy]

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.

v3: again, major rework, after long discussion with Max:
   - start creating graph looping through blk's and block jobs, don't use opaque
   - don't export pointers, generate ids instead
   (graphical representation didn't significantly changed, you can look at the
    picture, attached to v2 cover-letter)

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-debug-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           |  91 +++++++++++++++++++++++
 include/block/block.h          |   1 +
 include/sysemu/block-backend.h |   2 +
 block.c                        | 129 +++++++++++++++++++++++++++++++++
 block/block-backend.c          |   5 ++
 blockdev.c                     |   5 ++
 scripts/render_block_graph.py  | 120 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/222         |   2 +
 8 files changed, 355 insertions(+)
 create mode 100755 scripts/render_block_graph.py

-- 
2.18.0

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

[Vladimir Sementsov-Ogievskiy]

Add a new command, returning block nodes (and their users) graph.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json           |  91 +++++++++++++++++++++++
 include/block/block.h          |   1 +
 include/sysemu/block-backend.h |   2 +
 block.c                        | 129 +++++++++++++++++++++++++++++++++
 block/block-backend.c          |   5 ++
 blockdev.c                     |   5 ++
 6 files changed, 233 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4c7a37afdc..34cdc595d7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1629,6 +1629,97 @@
 ##
 { 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
 
+##
+# @BlockGraphNodeType:
+#
+# Since: 3.1
+##
+{ 'enum': 'BlockGraphNodeType',
+  'data': [ 'blk', 'job', 'bds' ] }
+
+##
+# @BlockGraphNode:
+#
+# Since: 3.1
+##
+{ 'struct': 'BlockGraphNode',
+  'data': { 'id': 'uint64', 'type': 'BlockGraphNodeType', 'name': 'str' } }
+
+##
+# @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-debug-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-debug-query-block-graph:
+#
+# Get the block graph.
+#
+# Since: 3.1
+##
+{ 'command': 'x-debug-query-block-graph', 'returns': 'BlockGraph' }
+
 ##
 # @drive-mirror:
 #
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/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 830d873f24..32ca5c1b12 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -237,4 +237,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
                                    int bytes, BdrvRequestFlags read_flags,
                                    BdrvRequestFlags write_flags);
 
+const BdrvChild *blk_root(BlockBackend *blk);
+
 #endif
diff --git a/block.c b/block.c
index 6161dbe3eb..588f5a2648 100644
--- a/block.c
+++ b/block.c
@@ -4003,6 +4003,135 @@ 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)
+
+typedef struct BlockGraphConstructor {
+    BlockGraph *graph;
+    GHashTable *hash;
+} BlockGraphConstructor;
+
+static BlockGraphConstructor *graph_new(void)
+{
+    BlockGraphConstructor *gr = g_new(BlockGraphConstructor, 1);
+
+    gr->graph = g_new0(BlockGraph, 1);
+    gr->hash = g_hash_table_new(NULL, NULL);
+
+    return gr;
+}
+
+static BlockGraph *graph_finalize(BlockGraphConstructor *gr)
+{
+    g_hash_table_destroy(gr->hash);
+
+    return gr->graph;
+}
+
+static uint64_t graph_node_num(BlockGraphConstructor *gr, void *node)
+{
+    uint64_t ret = (uint64_t)g_hash_table_lookup(gr->hash, node);
+
+    if (ret > 0) {
+        return ret;
+    }
+
+    ret = g_hash_table_size(gr->hash) + 1;
+    g_hash_table_insert(gr->hash, node, (void *)ret);
+
+    return ret;
+}
+
+static void graph_add_node(BlockGraphConstructor *gr, void *node,
+                           BlockGraphNodeType type, const char *name)
+{
+    BlockGraphNode *n;
+
+    n = g_new0(BlockGraphNode, 1);
+
+    n->id = graph_node_num(gr, node);
+    n->type = type;
+    n->name = g_strdup(name);
+
+    QAPI_LIST_ADD(gr->graph->nodes, n);
+}
+
+static void graph_add_edge(BlockGraphConstructor *gr, void *parent,
+                           const BdrvChild *child)
+{
+    typedef struct {
+        unsigned int flag;
+        BlockPermission num;
+    } PermissionMap;
+
+    static PermissionMap 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 }
+    };
+    PermissionMap *p;
+    BlockGraphEdge *edge;
+
+    edge = g_new0(BlockGraphEdge, 1);
+
+    edge->parent = graph_node_num(gr, parent);
+    edge->child = graph_node_num(gr, 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(gr->graph->edges, edge);
+}
+
+
+BlockGraph *bdrv_get_block_graph(Error **errp)
+{
+    BlockBackend *blk;
+    BlockJob *job;
+    BlockDriverState *bs;
+    BdrvChild *child;
+    BlockGraphConstructor *gr = graph_new();
+
+    for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) {
+        graph_add_node(gr, blk, BLOCK_GRAPH_NODE_TYPE_BLK, blk_name(blk));
+        if (blk_root(blk)) {
+            graph_add_edge(gr, blk, blk_root(blk));
+        }
+    }
+
+    for (job = block_job_next(NULL); job; job = block_job_next(job)) {
+        GSList *el;
+
+        graph_add_node(gr, job, BLOCK_GRAPH_NODE_TYPE_JOB, job->job.id);
+        for (el = job->nodes; el; el = el->next) {
+            graph_add_edge(gr, job, (BdrvChild *)el->data);
+        }
+    }
+
+    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
+        graph_add_node(gr, bs, BLOCK_GRAPH_NODE_TYPE_BDS, bs->node_name);
+        QLIST_FOREACH(child, &bs->children, next) {
+            graph_add_edge(gr, bs, child);
+        }
+    }
+
+    return graph_finalize(gr);
+}
+
 BlockDriverState *bdrv_lookup_bs(const char *device,
                                  const char *node_name,
                                  Error **errp)
diff --git a/block/block-backend.c b/block/block-backend.c
index fa120630be..f3704fe87d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2234,3 +2234,8 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
                               blk_out->root, off_out,
                               bytes, read_flags, write_flags);
 }
+
+const BdrvChild *blk_root(BlockBackend *blk)
+{
+    return blk->root;
+}
diff --git a/blockdev.c b/blockdev.c
index 72f5347df5..3b98ce7571 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_debug_query_block_graph(Error **errp)
+{
+    return bdrv_get_block_graph(errp);
+}
+
 BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
                              Error **errp)
 {
-- 
2.18.0

[Qemu-devel] [PATCH v3 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 | 120 ++++++++++++++++++++++++++++++++++
 1 file changed, 120 insertions(+)
 create mode 100755 scripts/render_block_graph.py

diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
new file mode 100755
index 0000000000..382cc769ef
--- /dev/null
+++ b/scripts/render_block_graph.py
@@ -0,0 +1,120 @@
+#!/usr/bin/env python
+#
+# Render Qemu Block Graph
+#
+# Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
+#
+# 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
+import sys
+import subprocess
+import json
+from graphviz import Digraph
+from qemu import MonitorResponseError
+
+
+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(qmp, filename, format='png'):
+    '''
+    Render graph in text (dot) representation into "@filename" and
+    representation in @format into "@filename.@format"
+    '''
+
+    bds_nodes = qmp.command('query-named-block-nodes')
+    bds_nodes = {n['node-name']: n for n in bds_nodes}
+
+    job_nodes = qmp.command('query-block-jobs')
+    job_nodes = {n['device']: n for n in job_nodes}
+
+    block_graph = qmp.command('x-debug-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'] == 'bds':
+            info = bds_nodes[n['name']]
+            label = n['name'] + ' [' + info['drv'] + ']'
+            if info['drv'] == 'file':
+                label += '\n' + os.path.basename(info['file'])
+            shape = 'ellipse'
+        elif n['type'] == 'job':
+            info = job_nodes[n['name']]
+            label = info['type'] + ' job (' + n['name'] + ')'
+            shape = 'box'
+        else:
+            assert n['type'] == 'blk'
+            label = n['name'] if n['name'] else 'unnamed blk'
+            shape = 'box'
+
+        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)
+
+
+class LibvirtGuest():
+    def __init__(self, name):
+        self.name = name
+
+    def command(self, cmd):
+        # only supports qmp commands without parameters
+        m = {'execute': cmd}
+        ar = ['virsh', 'qemu-monitor-command', self.name, json.dumps(m)]
+
+        reply = json.loads(subprocess.check_output(ar))
+
+        if 'error' in reply:
+            raise MonitorResponseError(reply)
+
+        return reply['return']
+
+
+if __name__ == '__main__':
+    obj = sys.argv[1]
+    out = sys.argv[2]
+
+    if os.path.exists(obj):
+        # assume unix socket
+        qmp = QEMUMonitorProtocol(obj)
+        qmp.connect()
+    else:
+        # assume libvirt guest name
+        qmp = LibvirtGuest(obj)
+
+    render_block_graph(qmp, out)
-- 
2.18.0

[Qemu-devel] [PATCH v3 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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
index 0ead56d574..91d88aa5c0 100644
--- a/tests/qemu-iotests/222
+++ b/tests/qemu-iotests/222
@@ -137,6 +137,8 @@ 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')
     log(vm.qmp('block-job-cancel', device=src_node))
     log(vm.event_wait('BLOCK_JOB_CANCELLED'),
         filters=[iotests.filter_qmp_event])
-- 
2.18.0

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

[Eduardo Habkost]

On Thu, Aug 23, 2018 at 06:46:54PM +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 | 120 ++++++++++++++++++++++++++++++++++
>  1 file changed, 120 insertions(+)
>  create mode 100755 scripts/render_block_graph.py
> 
> diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
> new file mode 100755
> index 0000000000..382cc769ef
> --- /dev/null
> +++ b/scripts/render_block_graph.py
> @@ -0,0 +1,120 @@
> +#!/usr/bin/env python
> +#
> +# Render Qemu Block Graph
> +#
> +# Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
> +#
> +# 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
> +import sys
> +import subprocess
> +import json
> +from graphviz import Digraph
> +from qemu import MonitorResponseError
> +
> +
> +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(qmp, filename, format='png'):
> +    '''
> +    Render graph in text (dot) representation into "@filename" and
> +    representation in @format into "@filename.@format"
> +    '''
> +
> +    bds_nodes = qmp.command('query-named-block-nodes')
> +    bds_nodes = {n['node-name']: n for n in bds_nodes}
> +
> +    job_nodes = qmp.command('query-block-jobs')
> +    job_nodes = {n['device']: n for n in job_nodes}
> +
> +    block_graph = qmp.command('x-debug-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'] == 'bds':
> +            info = bds_nodes[n['name']]
> +            label = n['name'] + ' [' + info['drv'] + ']'
> +            if info['drv'] == 'file':
> +                label += '\n' + os.path.basename(info['file'])
> +            shape = 'ellipse'
> +        elif n['type'] == 'job':
> +            info = job_nodes[n['name']]
> +            label = info['type'] + ' job (' + n['name'] + ')'
> +            shape = 'box'
> +        else:
> +            assert n['type'] == 'blk'
> +            label = n['name'] if n['name'] else 'unnamed blk'
> +            shape = 'box'
> +
> +        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)
> +
> +
> +class LibvirtGuest():
> +    def __init__(self, name):
> +        self.name = name
> +
> +    def command(self, cmd):
> +        # only supports qmp commands without parameters
> +        m = {'execute': cmd}
> +        ar = ['virsh', 'qemu-monitor-command', self.name, json.dumps(m)]
> +
> +        reply = json.loads(subprocess.check_output(ar))
> +
> +        if 'error' in reply:
> +            raise MonitorResponseError(reply)
> +
> +        return reply['return']

Interesting trick.  :)

Suggestion for a follow-up patch: we could move this helper class
to qmp.py, so other scripts could use it too.

> +
> +
> +if __name__ == '__main__':
> +    obj = sys.argv[1]
> +    out = sys.argv[2]
> +
> +    if os.path.exists(obj):
> +        # assume unix socket
> +        qmp = QEMUMonitorProtocol(obj)
> +        qmp.connect()
> +    else:
> +        # assume libvirt guest name
> +        qmp = LibvirtGuest(obj)
> +
> +    render_block_graph(qmp, out)
> -- 
> 2.18.0
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Thu, Aug 23, 2018 at 06:46:54PM +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>

Acked-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

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

[Max Reitz]

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

On 23.08.18 17:46, 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 | 120 ++++++++++++++++++++++++++++++++++
>  1 file changed, 120 insertions(+)
>  create mode 100755 scripts/render_block_graph.py

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

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

[Vladimir Sementsov-Ogievskiy]

28.09.2018 19:31, Max Reitz wrote:
> On 23.08.18 17:46, Vladimir Sementsov-Ogievskiy wrote:
>> Add a new command, returning block nodes (and their users) graph.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json           |  91 +++++++++++++++++++++++
>>   include/block/block.h          |   1 +
>>   include/sysemu/block-backend.h |   2 +
>>   block.c                        | 129 +++++++++++++++++++++++++++++++++
>>   block/block-backend.c          |   5 ++
>>   blockdev.c                     |   5 ++
>>   6 files changed, 233 insertions(+)
> The design looks better (that is to say, good) to me, so I mostly have
> technical remarks.  (Only a bit of bike-shedding this time.)
>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 4c7a37afdc..34cdc595d7 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1629,6 +1629,97 @@
>>   ##
>>   { 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
>>   
>> +##
>> +# @BlockGraphNodeType:
>> +#
>> +# Since: 3.1
>> +##
>> +{ 'enum': 'BlockGraphNodeType',
>> +  'data': [ 'blk', 'job', 'bds' ] }
> I wouldn't use abbreviations here, but the full names.  At least they
> should be described.

Hmm do you have a suggestion? Do you mean something like
   block-backend
   block-job
   block-driver-state
?


>
> (Though with x-debug-, it doesn't matter too much.)
>
>> +
>> +##
>> +# @BlockGraphNode:
>> +#
> I think a description on at least what the name means for each type
> would be useful; and that @id is generated just for this request and not
> some significant value in the block layer.

let me compose here, before sending next version:

@id: Block graph node identifier. This @id is generated only for 
x-debug-query-block-graph and don't relate to any other identifiers in Qemu.
@type: Type of graph node. Can be one of block-backend, block-job or 
block-driver-state.
@name: Human readable name of the node. Corresponds to node-name for 
block-driver-state nodes, and not guaranteed to be unique in the whole 
graph (with block-jobs and block-backends)

>
>> +# Since: 3.1
>> +##
>> +{ 'struct': 'BlockGraphNode',
>> +  'data': { 'id': 'uint64', 'type': 'BlockGraphNodeType', 'name': 'str' } }
>> +
>> +##
>> +# @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-debug-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
> s/attached this/attached to this/
>
>> +#
>> +# 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-debug-query-block-graph:
>> +#
>> +# Get the block graph.
>> +#
>> +# Since: 3.1
>> +##
>> +{ 'command': 'x-debug-query-block-graph', 'returns': 'BlockGraph' }
>> +
>>   ##
>>   # @drive-mirror:
>>   #
> [...]
>
>> diff --git a/block.c b/block.c
>> index 6161dbe3eb..588f5a2648 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4003,6 +4003,135 @@ 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; \
> Interesting, how yoo put (list) as an rvalue in parentheses, but don't
> do so when it's an lvalue. :-)
>
> I can follow your line of thinking, but it should probably always be in
> parentheses.

agree.

>
>> +} while (0)
>> +
>> +typedef struct BlockGraphConstructor {
>> +    BlockGraph *graph;
>> +    GHashTable *hash;
> Maybe...  "graph_nodes"?  "hash" seems awfully unspecific.

ok

>
>> +} BlockGraphConstructor;
>> +
>> +static BlockGraphConstructor *graph_new(void)
> I think this is a very broad name.  Maybe prefix all of the function
> names with debug_?  (Or dbg_, or dbg_blk, or something)

ok

>
>> +{
>> +    BlockGraphConstructor *gr = g_new(BlockGraphConstructor, 1);
>> +
>> +    gr->graph = g_new0(BlockGraph, 1);
>> +    gr->hash = g_hash_table_new(NULL, NULL);
>> +
>> +    return gr;
>> +}
>> +
>> +static BlockGraph *graph_finalize(BlockGraphConstructor *gr)
>> +{
>> +    g_hash_table_destroy(gr->hash);
>> +
>> +    return gr->graph;
> gr is leaked here.

good catch, will fix.

>
>> +}
>> +
>> +static uint64_t graph_node_num(BlockGraphConstructor *gr, void *node)
>> +{
>> +    uint64_t ret = (uint64_t)g_hash_table_lookup(gr->hash, node);
> I'd prefer a cast to uintptr_t.  Otherwise the compiler may warn that
> you cast a pointer to an integer of different size (with -m32).
>
> Storing it in a uint64_t (with an implicit cast then) is OK, though.
> But maybe you do want to store it in a uintptr_t.  The only reason not
> to is because it's a uint64_t in the QAPI schema, but I think it'd be a
> bit cleaner to work with uintptr_t internally and then emit it as
> uint64_t (because that's definitely safe).  It's just a bit more honest
> because this way it's clear that with -m32, we cannot even represent IDs
> larger than 32 bit (which doesn't matter).

ok. then, why not use just "void *" ?

>
>> +
>> +    if (ret > 0) {
> Just for style I'd prefer != over >.  That makes it more clear that this
> is a NULL check and not a check for errors (represented as negative
> integers, even though @ret is unsigned).

hm and it will be more clear with pointer type...

>
>> +        return ret;
>> +    }
>> +
>> +    ret = g_hash_table_size(gr->hash) + 1;
> Maybe add a comment that you add 1 because 0 (NULL) is reserved for
> non-entries?  (Yes, it's clear, or I wouldn't have figured it out, but
> I'd still find it nice.)

hmm, I don't remember why is it reserved, looks like it doesn't matter.. 
But it may be more native to count graph nodes from 1, not from 0.

>
>> +    g_hash_table_insert(gr->hash, node, (void *)ret);
> This definitely needs a uintptr_t cast or you'll get a warning for 32
> bit pointers.
>
>> +
>> +    return ret;
>> +}
>> +
>> +static void graph_add_node(BlockGraphConstructor *gr, void *node,
>> +                           BlockGraphNodeType type, const char *name)
>> +{
>> +    BlockGraphNode *n;
>> +
>> +    n = g_new0(BlockGraphNode, 1);
>> +
>> +    n->id = graph_node_num(gr, node);
>> +    n->type = type;
>> +    n->name = g_strdup(name);
>> +
>> +    QAPI_LIST_ADD(gr->graph->nodes, n);
>> +}
>> +
>> +static void graph_add_edge(BlockGraphConstructor *gr, void *parent,
>> +                           const BdrvChild *child)
>> +{
>> +    typedef struct {
>> +        unsigned int flag;
>> +        BlockPermission num;
>> +    } PermissionMap;
>> +
>> +    static PermissionMap permissions[] = {
> You can add a const if you like.

yes

>
>> +        { 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 }
>> +    };
> You could add a static assertion that "1 << (sizeof(permissions) /
> sizeof(permissions[0]) - 1) == BLK_PERM_ALL + 1" to ensure that whenever
> we add a permission, we don't forget to update this list.  But then
> again, I don't think we're going to add a permission any time soon...

ok.
hmm, we have cheating bdrv_mirror_top_child_perm(), which definitely 
shows that permission scheme is not complete.. :)

>
>> +    PermissionMap *p;
>> +    BlockGraphEdge *edge;
>> +
>> +    edge = g_new0(BlockGraphEdge, 1);
>> +
>> +    edge->parent = graph_node_num(gr, parent);
>> +    edge->child = graph_node_num(gr, 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(gr->graph->edges, edge);
>> +}
>> +
>> +
>> +BlockGraph *bdrv_get_block_graph(Error **errp)
>> +{
>> +    BlockBackend *blk;
>> +    BlockJob *job;
>> +    BlockDriverState *bs;
>> +    BdrvChild *child;
>> +    BlockGraphConstructor *gr = graph_new();
>> +
>> +    for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) {
>> +        graph_add_node(gr, blk, BLOCK_GRAPH_NODE_TYPE_BLK, blk_name(blk));
> Hm, I think blk_name() is empty for most backends.
> blk_root_get_parent_desc() then falls back to blk_get_attached_dev_id().
>   I think we should do the same here.

ok, good thing. However backup target blk will remain unnamed anyway.

>
> Max
>
>> +        if (blk_root(blk)) {
>> +            graph_add_edge(gr, blk, blk_root(blk));
>> +        }
>> +    }
>> +
>> +    for (job = block_job_next(NULL); job; job = block_job_next(job)) {
>> +        GSList *el;
>> +
>> +        graph_add_node(gr, job, BLOCK_GRAPH_NODE_TYPE_JOB, job->job.id);
>> +        for (el = job->nodes; el; el = el->next) {
>> +            graph_add_edge(gr, job, (BdrvChild *)el->data);
>> +        }
>> +    }
>> +
>> +    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
>> +        graph_add_node(gr, bs, BLOCK_GRAPH_NODE_TYPE_BDS, bs->node_name);
>> +        QLIST_FOREACH(child, &bs->children, next) {
>> +            graph_add_edge(gr, bs, child);
>> +        }
>> +    }
>> +
>> +    return graph_finalize(gr);
>> +}
>> +
>>   BlockDriverState *bdrv_lookup_bs(const char *device,
>>                                    const char *node_name,
>>                                    Error **errp)


-- 
Best regards,
Vladimir

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

[Max Reitz]

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

On 02.10.18 15:01, Vladimir Sementsov-Ogievskiy wrote:
> 28.09.2018 19:31, Max Reitz wrote:
>> On 23.08.18 17:46, Vladimir Sementsov-Ogievskiy wrote:
>>> Add a new command, returning block nodes (and their users) graph.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   qapi/block-core.json           |  91 +++++++++++++++++++++++
>>>   include/block/block.h          |   1 +
>>>   include/sysemu/block-backend.h |   2 +
>>>   block.c                        | 129 +++++++++++++++++++++++++++++++++
>>>   block/block-backend.c          |   5 ++
>>>   blockdev.c                     |   5 ++
>>>   6 files changed, 233 insertions(+)
>> The design looks better (that is to say, good) to me, so I mostly have
>> technical remarks.  (Only a bit of bike-shedding this time.)
>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 4c7a37afdc..34cdc595d7 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1629,6 +1629,97 @@
>>>   ##
>>>   { 'command': 'query-named-block-nodes', 'returns': [
>>> 'BlockDeviceInfo' ] }
>>>   +##
>>> +# @BlockGraphNodeType:
>>> +#
>>> +# Since: 3.1
>>> +##
>>> +{ 'enum': 'BlockGraphNodeType',
>>> +  'data': [ 'blk', 'job', 'bds' ] }
>> I wouldn't use abbreviations here, but the full names.  At least they
>> should be described.
> 
> Hmm do you have a suggestion? Do you mean something like
>   block-backend
>   block-job
>   block-driver-state
> ?

That's what I meant, yes; though I'm not sure I was right.

"block-backend" sounds good; "job" is up to you since it isn't
necessarily an abbreviation.

About BDSs...  I guess I'll leave it up to you, too.  block-driver-state
doesn't tell people more than just "bds", so it isn't much more useful.
 Usually I'd call it "node" because that's what we like to call it in
docs.  Unfortunately, that won't quite work here, because in this case,
all of these things are nodes.

"block-node" would be the best I can come up with.  But I'm not sure
whether it's any better than "bds", so I'll just leave it up to you.

Documentation would still be nice, but I don't even know how you'd
describe @bds.  "BlockDriverState"?  Technically it'd be "Node of the
block graph", but, er, well.  That's a bad description for an element of
the "BlockGraphNodeType" enum.

So what I'm saying is "block-backend" would be nice, the rest is up to you.

>> (Though with x-debug-, it doesn't matter too much.)
>>
>>> +
>>> +##
>>> +# @BlockGraphNode:
>>> +#
>> I think a description on at least what the name means for each type
>> would be useful; and that @id is generated just for this request and not
>> some significant value in the block layer.
> 
> let me compose here, before sending next version:
> 
> @id: Block graph node identifier. This @id is generated only for
> x-debug-query-block-graph and don't relate to any other identifiers in

s/don't/does not/

> Qemu.
> @type: Type of graph node. Can be one of block-backend, block-job or
> block-driver-state.
> @name: Human readable name of the node. Corresponds to node-name for
> block-driver-state nodes, and not guaranteed to be unique in the whole

I'd prefer s/, and/; is/ (or s/, and/. Is/)

> graph (with block-jobs and block-backends)

Sounds good to me overall.

[...]

>>> +}
>>> +
>>> +static uint64_t graph_node_num(BlockGraphConstructor *gr, void *node)
>>> +{
>>> +    uint64_t ret = (uint64_t)g_hash_table_lookup(gr->hash, node);
>> I'd prefer a cast to uintptr_t.  Otherwise the compiler may warn that
>> you cast a pointer to an integer of different size (with -m32).
>>
>> Storing it in a uint64_t (with an implicit cast then) is OK, though.
>> But maybe you do want to store it in a uintptr_t.  The only reason not
>> to is because it's a uint64_t in the QAPI schema, but I think it'd be a
>> bit cleaner to work with uintptr_t internally and then emit it as
>> uint64_t (because that's definitely safe).  It's just a bit more honest
>> because this way it's clear that with -m32, we cannot even represent IDs
>> larger than 32 bit (which doesn't matter).
> 
> ok. then, why not use just "void *" ?

Because you want to store integer IDs and not pointers.

>>> +
>>> +    if (ret > 0) {
>> Just for style I'd prefer != over >.  That makes it more clear that this
>> is a NULL check and not a check for errors (represented as negative
>> integers, even though @ret is unsigned).
> 
> hm and it will be more clear with pointer type...
> 
>>
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = g_hash_table_size(gr->hash) + 1;
>> Maybe add a comment that you add 1 because 0 (NULL) is reserved for
>> non-entries?  (Yes, it's clear, or I wouldn't have figured it out, but
>> I'd still find it nice.)
> 
> hmm, I don't remember why is it reserved, looks like it doesn't matter..
> But it may be more native to count graph nodes from 1, not from 0.

Because you only have a single function for adding entries to and
querying entries from gr->hash.  So it needs to distinguish whether a
node already has an ID or not.  If it does not, g_hash_table_lookup()
will return NULL, so you cannot add entries with index 0, because they
will look like the entry does not exist yet.  (So the first call to
graph_node_num() will return 0, and the second call for the same node
will overwrite that value with something else.  Presumably you'll then
have a BlockBackend with ID 0 but not edge using the ID 0.)

You can use ID 0 if instead of checking whether "ret != 0" you'd use
g_hash_table_contains() or g_hash_table_lookup_extended() instead.

Max


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

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

[Vladimir Sementsov-Ogievskiy]

05.10.2018 22:34, Max Reitz wrote:
> On 02.10.18 15:01, Vladimir Sementsov-Ogievskiy wrote:
>> 28.09.2018 19:31, Max Reitz wrote:
>>> On 23.08.18 17:46, Vladimir Sementsov-Ogievskiy wrote:
>>>> Add a new command, returning block nodes (and their users) graph.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    qapi/block-core.json           |  91 +++++++++++++++++++++++
>>>>    include/block/block.h          |   1 +
>>>>    include/sysemu/block-backend.h |   2 +
>>>>    block.c                        | 129 +++++++++++++++++++++++++++++++++
>>>>    block/block-backend.c          |   5 ++
>>>>    blockdev.c                     |   5 ++
>>>>    6 files changed, 233 insertions(+)
>>> The design looks better (that is to say, good) to me, so I mostly have
>>> technical remarks.  (Only a bit of bike-shedding this time.)
>>>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 4c7a37afdc..34cdc595d7 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -1629,6 +1629,97 @@
>>>>    ##
>>>>    { 'command': 'query-named-block-nodes', 'returns': [
>>>> 'BlockDeviceInfo' ] }
>>>>    +##
>>>> +# @BlockGraphNodeType:
>>>> +#
>>>> +# Since: 3.1
>>>> +##
>>>> +{ 'enum': 'BlockGraphNodeType',
>>>> +  'data': [ 'blk', 'job', 'bds' ] }
>>> I wouldn't use abbreviations here, but the full names.  At least they
>>> should be described.
>> Hmm do you have a suggestion? Do you mean something like
>>    block-backend
>>    block-job
>>    block-driver-state
>> ?
> That's what I meant, yes; though I'm not sure I was right.
>
> "block-backend" sounds good; "job" is up to you since it isn't
> necessarily an abbreviation.
>
> About BDSs...  I guess I'll leave it up to you, too.  block-driver-state
> doesn't tell people more than just "bds", so it isn't much more useful.
>   Usually I'd call it "node" because that's what we like to call it in
> docs.  Unfortunately, that won't quite work here, because in this case,
> all of these things are nodes.
>
> "block-node" would be the best I can come up with.  But I'm not sure
> whether it's any better than "bds", so I'll just leave it up to you.
>
> Documentation would still be nice, but I don't even know how you'd
> describe @bds.  "BlockDriverState"?  Technically it'd be "Node of the
> block graph", but, er, well.  That's a bad description for an element of
> the "BlockGraphNodeType" enum.
>
> So what I'm saying is "block-backend" would be nice, the rest is up to you.
>
>>> (Though with x-debug-, it doesn't matter too much.)
>>>
>>>> +
>>>> +##
>>>> +# @BlockGraphNode:
>>>> +#
>>> I think a description on at least what the name means for each type
>>> would be useful; and that @id is generated just for this request and not
>>> some significant value in the block layer.
>> let me compose here, before sending next version:
>>
>> @id: Block graph node identifier. This @id is generated only for
>> x-debug-query-block-graph and don't relate to any other identifiers in
> s/don't/does not/
>
>> Qemu.
>> @type: Type of graph node. Can be one of block-backend, block-job or
>> block-driver-state.
>> @name: Human readable name of the node. Corresponds to node-name for
>> block-driver-state nodes, and not guaranteed to be unique in the whole
> I'd prefer s/, and/; is/ (or s/, and/. Is/)
>
>> graph (with block-jobs and block-backends)
> Sounds good to me overall.
>
> [...]
>
>>>> +}
>>>> +
>>>> +static uint64_t graph_node_num(BlockGraphConstructor *gr, void *node)
>>>> +{
>>>> +    uint64_t ret = (uint64_t)g_hash_table_lookup(gr->hash, node);
>>> I'd prefer a cast to uintptr_t.  Otherwise the compiler may warn that
>>> you cast a pointer to an integer of different size (with -m32).
>>>
>>> Storing it in a uint64_t (with an implicit cast then) is OK, though.
>>> But maybe you do want to store it in a uintptr_t.  The only reason not
>>> to is because it's a uint64_t in the QAPI schema, but I think it'd be a
>>> bit cleaner to work with uintptr_t internally and then emit it as
>>> uint64_t (because that's definitely safe).  It's just a bit more honest
>>> because this way it's clear that with -m32, we cannot even represent IDs
>>> larger than 32 bit (which doesn't matter).
>> ok. then, why not use just "void *" ?
> Because you want to store integer IDs and not pointers.

Ah, didn't know what is uintptr_t, ashamed)

>
>>>> +
>>>> +    if (ret > 0) {
>>> Just for style I'd prefer != over >.  That makes it more clear that this
>>> is a NULL check and not a check for errors (represented as negative
>>> integers, even though @ret is unsigned).
>> hm and it will be more clear with pointer type...
>>
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    ret = g_hash_table_size(gr->hash) + 1;
>>> Maybe add a comment that you add 1 because 0 (NULL) is reserved for
>>> non-entries?  (Yes, it's clear, or I wouldn't have figured it out, but
>>> I'd still find it nice.)
>> hmm, I don't remember why is it reserved, looks like it doesn't matter..
>> But it may be more native to count graph nodes from 1, not from 0.
> Because you only have a single function for adding entries to and
> querying entries from gr->hash.  So it needs to distinguish whether a
> node already has an ID or not.  If it does not, g_hash_table_lookup()
> will return NULL, so you cannot add entries with index 0, because they
> will look like the entry does not exist yet.  (So the first call to
> graph_node_num() will return 0, and the second call for the same node
> will overwrite that value with something else.  Presumably you'll then
> have a BlockBackend with ID 0 but not edge using the ID 0.)
>
> You can use ID 0 if instead of checking whether "ret != 0" you'd use
> g_hash_table_contains() or g_hash_table_lookup_extended() instead.

Hm, looks correct. It's a bit strange to thank for explanation of my own 
code, nevertheless thank you!

>
> Max
>


-- 
Best regards,
Vladimir