qemu-block
[Top][All Lists]
Advanced

[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 21:38:13 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

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

+#
+# @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




reply via email to

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