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

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
Good point, agree.

So I'd prefer using e.g. a hash map to map pointers to freshly
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

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


Best regards,

reply via email to

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