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

[Vladimir Sementsov-Ogievskiy]

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
> > > > > x-debug-.
> > > > Good point, agree.
> > > >
> > > > > So I'd prefer using e.g. a hash map to map pointers to freshly
> > > > > generated
> > > > > 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
> anyway).
>
> 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..

> Max


--
Best regards,
Vladimir