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 <vsement...@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
