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
