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

[Max Reitz]

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 <address@hidden>
>>>> ---
>>>>    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