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