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

[Vladimir Sementsov-Ogievskiy]

17.08.2018 23: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).

I thought about it, but there no corresponding enum in Qemu too, so it's 
like driver name in query-block.. However, I can
create a function which will map &role pointer to corresponding qapi 
enum element, which will need to be updated when new
roles created. Not sure that it's better.

> > +#
> > +# @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?

Hmm, just a new command, used mostly for debugging. I don't have any 
special plans on improving it, I'm ok to drop x- prefix.

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

Yes, we should decide about enum.. If we want a qapi enum, how to mirror 
it in internal qemu structures better? Only by mapping function (ptr -> 
enum-element), or may be add field of EnumType entirely into the role 
structure?

> > +++ 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?

Agree.

> > +
> > +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?

Internal is not enum, they are flags.. How can we use enum instead? Or I 
don't understand what you mean..

> > +        { 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).

Yes, I'm for viewing maximum number of nodes.. However my code goes 
through graph_bdrv_stats, query-named-block-nodes do the same.. Isn't it 
the whole set of nodes? All named... But, as I understand, now all nodes 
are named?

--
Best regards,
Vladimir