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 <vsement...@virtuozzo.com> > --- > 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