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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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