On 2018-08-20 20:38, Vladimir Sementsov-Ogievskiy wrote:
> 20.08.2018 16:44, Max Reitz wrote:
>> On 2018-08-20 12:20, Vladimir Sementsov-Ogievskiy wrote:
>>> 18.08.2018 00:03, Max Reitz wrote:
>>>> On 2018-08-17 22:32, Eric Blake wrote:
>>>>> On 08/17/2018 01:04 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Add a new command, returning block nodes graph.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>>>>> <vsement...@virtuozzo.com>
>>>>>> ---
>>>>>> qapi/block-core.json | 116
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> +##
>>>>>> +# @BlockGraphEdge:
>>>>>> +#
>>>>>> +# Block Graph edge description for x-query-block-graph.
>>>>>> +#
>>>>>> +# @parent: parent id
>>>>>> +#
>>>>>> +# @child: child id
>>>>>> +#
>>>>>> +# @name: name of the relation (examples are 'file' and 'backing')
>>>>> Can this be made into a QAPI enum? (It would have ripple effects to
>>>>> existing code, but might be a worthwhile cleanup).
>>>>>
>>>>>> +#
>>>>>> +# @perm: granted permissions for the parent operating on the child
>>>>>> +#
>>>>>> +# @shared-perm: permissions that can still be granted to other users
>>>>>> of the
>>>>>> +# child while it is still attached this parent
>>>>>> +#
>>>>>> +# Since: 3.1
>>>>>> +##
>>>>>> +{ 'struct': 'BlockGraphEdge',
>>>>>> + 'data': { 'parent': 'uint64', 'child': 'uint64',
>>>>>> + 'name': 'str', 'perm': [ 'BlockPermission' ],
>>>>>> + 'shared-perm': [ 'BlockPermission' ] } }
>>>>>> +
>>>>>> +##
>>>>>> +# @x-query-block-graph:
>>>>>> +#
>>>>>> +# Get the block graph.
>>>>>> +#
>>>>>> +# Since: 3.1
>>>>>> +##
>>>>>> +{ 'command': 'x-query-block-graph', 'returns': 'BlockGraph' }
>>>>> Why is this command given an x- prefix? What would it take to promote
>>>>> it from experimental to fully supported?
>>>> This is just a very bad reasons, but I'll give it anyway: We really
>>>> want
>>>> such a command but still don't have one. So I doubt this is exactly
>>>> what we want. :-)
>>>>
>>>> A better reason is that we probably do not want a single command to
>>>> return the whole block graph. Do we want the command to just return
>>>> info for a single node, including just the node names of the children?
>>>> Do we want the command to include child info on request (but start from
>>>> a user-specified root)?
>>>>
>>>> Also, the interface is... Er, weird? Honestly, I don't quite see why
>>>> we would want it like this without x-.
>>>>
>>>> Why use newly generated IDs instead of node names? Why are those RAM
>>>> addresses? That is just so fishy.
>>>>
>>>> Because parents can be non-nodes? Well, I suppose you better not
>>>> include them in the graph like this, then.
>>>>
>>>> I don't see why the query command we want would include non-BDSs at
>>>> all.
>>>>
>>>> It may be useful for debugging, so, er, well, with an x-debug- prefix,
>>>> OK. But the question then is whether it's useful enough to warrant
>>>> having a separate query command that isn't precisely the command we
>>>> want
>>>> anyway.
>>>
>>>> The first question we probably have to ask is whether the query command
>>>> needs to output parent information. If not, querying would probably
>>>> start at some named node and then you'd go down from there (either with
>>>> many queries or through a single one).
>>>>
>>>> If so, well, then we can still output parent information, but I'd say
>>>> that then is purely informational and we don't need to "generate" new
>>>> IDs for them. Just have either a node-name there or a user-readable
>>>> description (like it was in v1; although it has to be noted that such a
>>>> user-readable description is useless to a management layer), but these
>>>> new IDs are really not something I like.
>>>>
>>>>> Overall, the command looks quite useful; and the fact that you
>>>>> produced
>>>>> some nice .dot graphs from it for visualization seems like it is worth
>>>>> considering this as a permanent API addition. The question, then,
>>>>> is if
>>>>> the interface is right, or if it needs any tweaks (like using an enum
>>>>> instead of open-coded string for the relation between parent and
>>>>> child),
>>>>> as a reason for keeping the x- prefix.
>>>> You can use x-debug- even when you decide to keep a command.
>>>>
>>>> I see no reason why a command should hastily not get an x- prefix just
>>>> because it may be useful enough. If it really is and we really see the
>>>> interface is good (which I really don't think it is), then we can
>>>> always
>>>> drop it later.
>>>>
>>>> Max
>>>>
>>>>>> +++ b/block.c
>>>>>> @@ -4003,6 +4003,86 @@ BlockDeviceInfoList
>>>>>> *bdrv_named_nodes_list(Error **errp)
>>>>>> return list;
>>>>>> }
>>>>>> +#define QAPI_LIST_ADD(list, element) do { \
>>>>>> + typeof(list) _tmp = g_new(typeof(*(list)), 1); \
>>>>>> + _tmp->value = (element); \
>>>>>> + _tmp->next = (list); \
>>>>>> + list = _tmp; \
>>>>>> +} while (0)
>>>>> Hmm - this seems like a frequently observed pattern - should it be
>>>>> something that we add independently and use throughout the tree as
>>>>> part
>>>>> of QAPI interactions in general?
>>>>>
>>>>>> +
>>>>>> +BlockGraph *bdrv_get_block_graph(Error **errp)
>>>>>> +{
>>>>>> + BlockGraph *graph = g_new0(BlockGraph, 1);
>>>>>> + BlockDriverState *bs;
>>>>>> + BdrvChild *child;
>>>>>> + BlockGraphNode *node;
>>>>>> + BlockGraphEdge *edge;
>>>>>> + struct {
>>>>>> + unsigned int flag;
>>>>>> + BlockPermission num;
>>>>>> + } permissions[] = {
>>>>>> + { BLK_PERM_CONSISTENT_READ,
>>>>>> BLOCK_PERMISSION_CONSISTENT_READ },
>>>>> Would it be worth directly reusing a QAPI enum for all such
>>>>> permissions
>>>>> in the existing code base, instead of having to map between an
>>>>> internal
>>>>> and a QAPI enum?
>>>>>
>>>>>> + { BLK_PERM_WRITE, BLOCK_PERMISSION_WRITE },
>>>>>> + { BLK_PERM_WRITE_UNCHANGED,
>>>>>> BLOCK_PERMISSION_WRITE_UNCHANGED },
>>>>>> + { BLK_PERM_RESIZE, BLOCK_PERMISSION_RESIZE },
>>>>>> + { BLK_PERM_GRAPH_MOD, BLOCK_PERMISSION_GRAPH_MOD },
>>>>>> + { 0, 0 }
>>>>>> + }, *p;
>>>>>> +
>>>>>> + QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
>>>>>> + QLIST_FOREACH(child, &bs->parents, next_parent) {
>>>>>> + if (!child->role->parent_is_bds) {
>>>>>> + BlockGraphNodeList *el;
>>>>>> + bool add = true;
>>>>> Does your command need/want to expose internal nodes? (I argue that it
>>>>> probably should show internal nodes, even when normal 'query-block'
>>>>> doesn't, just because knowing about the impact of internal nodes
>>>>> can be
>>>>> important when tracking down permissions issues).
>>>> You could make it a flag, but in theory, implicit nodes should never be
>>>> visible through QMP. (Though if you explicitly request them, that's
>>>> probably a different story.)
>>>>
>>>> Max
>>>>
>>> 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-.
>
> Hmm ASLR, what about trace points? Do they violate it? We can enable
> trace-points on running vm and they'll show a lot of pointers...Good question, but at least you can disable it during configure. If query-block-graph is disabled by default and needs to be enabled explicitly, I guess that'd be OK. Another benefit of using a hash map though would be that you don't need to loop through the array of existing nodes to find out which you have added already -- a lookup in the hash map would be enough. Max >> So I'd prefer using e.g. a hash map to map pointers to freshly generated >> IDs (just incrementing integers). >> >> 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. >> >> I suppose the alternative would be to start from all possible roots and >> generate the graph from there (all possible roots being all >> monitor-owned BDSs and all BlockBackends, I think). >> >> Max >> > >
signature.asc
Description: OpenPGP digital signature
