[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/3] qapi: add x-debug-query-block-graph
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/3] qapi: add x-debug-query-block-graph |
Date: |
Fri, 28 Sep 2018 18:31:53 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
On 23.08.18 17:46, Vladimir Sementsov-Ogievskiy wrote:
> Add a new command, returning block nodes (and their users) graph.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
> qapi/block-core.json | 91 +++++++++++++++++++++++
> include/block/block.h | 1 +
> include/sysemu/block-backend.h | 2 +
> block.c | 129 +++++++++++++++++++++++++++++++++
> block/block-backend.c | 5 ++
> blockdev.c | 5 ++
> 6 files changed, 233 insertions(+)
The design looks better (that is to say, good) to me, so I mostly have
technical remarks. (Only a bit of bike-shedding this time.)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4c7a37afdc..34cdc595d7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1629,6 +1629,97 @@
> ##
> { 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
>
> +##
> +# @BlockGraphNodeType:
> +#
> +# Since: 3.1
> +##
> +{ 'enum': 'BlockGraphNodeType',
> + 'data': [ 'blk', 'job', 'bds' ] }
I wouldn't use abbreviations here, but the full names. At least they
should be described.
(Though with x-debug-, it doesn't matter too much.)
> +
> +##
> +# @BlockGraphNode:
> +#
I think a description on at least what the name means for each type
would be useful; and that @id is generated just for this request and not
some significant value in the block layer.
> +# Since: 3.1
> +##
> +{ 'struct': 'BlockGraphNode',
> + 'data': { 'id': 'uint64', 'type': 'BlockGraphNodeType', 'name': 'str' } }
> +
> +##
> +# @BlockPermission:
> +#
> +# Enum of base block permissions.
> +#
> +# @consistent-read: A user that has the "permission" of consistent reads is
> +# guaranteed that their view of the contents of the block
> +# device is complete and self-consistent, representing the
> +# contents of a disk at a specific point.
> +# For most block devices (including their backing files)
> this
> +# is true, but the property cannot be maintained in a few
> +# situations like for intermediate nodes of a commit block
> +# job.
> +#
> +# @write: This permission is required to change the visible disk contents.
> +#
> +# @write-unchanged: This permission (which is weaker than BLK_PERM_WRITE) is
> +# both enough and required for writes to the block node
> when
> +# the caller promises that the visible disk content doesn't
> +# change.
> +# As the BLK_PERM_WRITE permission is strictly stronger,
> +# either is sufficient to perform an unchanging write.
> +#
> +# @resize: This permission is required to change the size of a block node.
> +#
> +# @graph-mod: This permission is required to change the node that this
> +# BdrvChild points to.
> +#
> +# Since: 3.1
> +##
> + { 'enum': 'BlockPermission',
> + 'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize',
> + 'graph-mod' ] }
> +##
> +# @BlockGraphEdge:
> +#
> +# Block Graph edge description for x-debug-query-block-graph.
> +#
> +# @parent: parent id
> +#
> +# @child: child id
> +#
> +# @name: name of the relation (examples are 'file' and 'backing')
> +#
> +# @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
s/attached this/attached to this/
> +#
> +# Since: 3.1
> +##
> +{ 'struct': 'BlockGraphEdge',
> + 'data': { 'parent': 'uint64', 'child': 'uint64',
> + 'name': 'str', 'perm': [ 'BlockPermission' ],
> + 'shared-perm': [ 'BlockPermission' ] } }
> +
> +##
> +# @BlockGraph:
> +#
> +# Block Graph - list of nodes and list of edges.
> +#
> +# Since: 3.1
> +##
> +{ 'struct': 'BlockGraph',
> + 'data': { 'nodes': ['BlockGraphNode'], 'edges': ['BlockGraphEdge'] } }
> +
> +##
> +# @x-debug-query-block-graph:
> +#
> +# Get the block graph.
> +#
> +# Since: 3.1
> +##
> +{ 'command': 'x-debug-query-block-graph', 'returns': 'BlockGraph' }
> +
> ##
> # @drive-mirror:
> #
[...]
> diff --git a/block.c b/block.c
> index 6161dbe3eb..588f5a2648 100644
> --- a/block.c
> +++ b/block.c
> @@ -4003,6 +4003,135 @@ 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; \
Interesting, how yoo put (list) as an rvalue in parentheses, but don't
do so when it's an lvalue. :-)
I can follow your line of thinking, but it should probably always be in
parentheses.
> +} while (0)
> +
> +typedef struct BlockGraphConstructor {
> + BlockGraph *graph;
> + GHashTable *hash;
Maybe... "graph_nodes"? "hash" seems awfully unspecific.
> +} BlockGraphConstructor;
> +
> +static BlockGraphConstructor *graph_new(void)
I think this is a very broad name. Maybe prefix all of the function
names with debug_? (Or dbg_, or dbg_blk, or something)
> +{
> + BlockGraphConstructor *gr = g_new(BlockGraphConstructor, 1);
> +
> + gr->graph = g_new0(BlockGraph, 1);
> + gr->hash = g_hash_table_new(NULL, NULL);
> +
> + return gr;
> +}
> +
> +static BlockGraph *graph_finalize(BlockGraphConstructor *gr)
> +{
> + g_hash_table_destroy(gr->hash);
> +
> + return gr->graph;
gr is leaked here.
> +}
> +
> +static uint64_t graph_node_num(BlockGraphConstructor *gr, void *node)
> +{
> + uint64_t ret = (uint64_t)g_hash_table_lookup(gr->hash, node);
I'd prefer a cast to uintptr_t. Otherwise the compiler may warn that
you cast a pointer to an integer of different size (with -m32).
Storing it in a uint64_t (with an implicit cast then) is OK, though.
But maybe you do want to store it in a uintptr_t. The only reason not
to is because it's a uint64_t in the QAPI schema, but I think it'd be a
bit cleaner to work with uintptr_t internally and then emit it as
uint64_t (because that's definitely safe). It's just a bit more honest
because this way it's clear that with -m32, we cannot even represent IDs
larger than 32 bit (which doesn't matter).
> +
> + if (ret > 0) {
Just for style I'd prefer != over >. That makes it more clear that this
is a NULL check and not a check for errors (represented as negative
integers, even though @ret is unsigned).
> + return ret;
> + }
> +
> + ret = g_hash_table_size(gr->hash) + 1;
Maybe add a comment that you add 1 because 0 (NULL) is reserved for
non-entries? (Yes, it's clear, or I wouldn't have figured it out, but
I'd still find it nice.)
> + g_hash_table_insert(gr->hash, node, (void *)ret);
This definitely needs a uintptr_t cast or you'll get a warning for 32
bit pointers.
> +
> + return ret;
> +}
> +
> +static void graph_add_node(BlockGraphConstructor *gr, void *node,
> + BlockGraphNodeType type, const char *name)
> +{
> + BlockGraphNode *n;
> +
> + n = g_new0(BlockGraphNode, 1);
> +
> + n->id = graph_node_num(gr, node);
> + n->type = type;
> + n->name = g_strdup(name);
> +
> + QAPI_LIST_ADD(gr->graph->nodes, n);
> +}
> +
> +static void graph_add_edge(BlockGraphConstructor *gr, void *parent,
> + const BdrvChild *child)
> +{
> + typedef struct {
> + unsigned int flag;
> + BlockPermission num;
> + } PermissionMap;
> +
> + static PermissionMap permissions[] = {
You can add a const if you like.
> + { BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ },
> + { 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 }
> + };
You could add a static assertion that "1 << (sizeof(permissions) /
sizeof(permissions[0]) - 1) == BLK_PERM_ALL + 1" to ensure that whenever
we add a permission, we don't forget to update this list. But then
again, I don't think we're going to add a permission any time soon...
> + PermissionMap *p;
> + BlockGraphEdge *edge;
> +
> + edge = g_new0(BlockGraphEdge, 1);
> +
> + edge->parent = graph_node_num(gr, parent);
> + edge->child = graph_node_num(gr, child->bs);
> + edge->name = g_strdup(child->name);
> +
> + for (p = permissions; p->flag; p++) {
> + if (p->flag & child->perm) {
> + QAPI_LIST_ADD(edge->perm, p->num);
> + }
> + if (p->flag & child->shared_perm) {
> + QAPI_LIST_ADD(edge->shared_perm, p->num);
> + }
> + }
> +
> + QAPI_LIST_ADD(gr->graph->edges, edge);
> +}
> +
> +
> +BlockGraph *bdrv_get_block_graph(Error **errp)
> +{
> + BlockBackend *blk;
> + BlockJob *job;
> + BlockDriverState *bs;
> + BdrvChild *child;
> + BlockGraphConstructor *gr = graph_new();
> +
> + for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) {
> + graph_add_node(gr, blk, BLOCK_GRAPH_NODE_TYPE_BLK, blk_name(blk));
Hm, I think blk_name() is empty for most backends.
blk_root_get_parent_desc() then falls back to blk_get_attached_dev_id().
I think we should do the same here.
Max
> + if (blk_root(blk)) {
> + graph_add_edge(gr, blk, blk_root(blk));
> + }
> + }
> +
> + for (job = block_job_next(NULL); job; job = block_job_next(job)) {
> + GSList *el;
> +
> + graph_add_node(gr, job, BLOCK_GRAPH_NODE_TYPE_JOB, job->job.id);
> + for (el = job->nodes; el; el = el->next) {
> + graph_add_edge(gr, job, (BdrvChild *)el->data);
> + }
> + }
> +
> + QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
> + graph_add_node(gr, bs, BLOCK_GRAPH_NODE_TYPE_BDS, bs->node_name);
> + QLIST_FOREACH(child, &bs->children, next) {
> + graph_add_edge(gr, bs, child);
> + }
> + }
> +
> + return graph_finalize(gr);
> +}
> +
> BlockDriverState *bdrv_lookup_bs(const char *device,
> const char *node_name,
> Error **errp)
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v3 1/3] qapi: add x-debug-query-block-graph,
Max Reitz <=