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

[Max Reitz]

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.

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.

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

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

Max

signature.asc
Description: OpenPGP digital signature