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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8875 bytes --]

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
>>>>>> <vsementsov@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
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]