[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH v2 1/3] qapi: add x-query-block-graph
Date: Mon, 20 Aug 2018 13:03:13 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

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 <address@hidden>
  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?


+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[] = {

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

+        { 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,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]