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

[Eric Blake]

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?

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.

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

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org