From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39341) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVBHS-0007Wf-0R for qemu-devel@nongnu.org; Mon, 15 Feb 2016 00:01:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aVBHQ-0003td-Lk for qemu-devel@nongnu.org; Mon, 15 Feb 2016 00:01:01 -0500 Message-ID: <56C15BBF.3010001@cn.fujitsu.com> Date: Mon, 15 Feb 2016 13:01:51 +0800 From: Changlong Xie MIME-Version: 1.0 References: <1451035376-7252-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1451035376-7252-4-git-send-email-xiecl.fnst@cn.fujitsu.com> <56BB7B30.8050501@redhat.com> In-Reply-To: <56BB7B30.8050501@redhat.com> Content-Type: text/plain; charset="iso-8859-15"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v9 3/3] qmp: add monitor command to add/remove a child List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu devel , Eric Blake , Alberto Garcia , Kevin Wolf , Stefan Hajnoczi Cc: qemu block , Jiang Yunhong , Dong Eddie , "Dr. David Alan Gilbert" , Markus Armbruster , Gonglei , zhanghailiang On 02/11/2016 02:02 AM, Max Reitz wrote: > On 25.12.2015 10:22, Changlong Xie wrote: >> From: Wen Congyang >> >> The new QMP command name is x-blockdev-change. It's just for adding/removing >> quorum's child now, and doesn't support all kinds of children, all kinds of >> operations, nor all block drivers. So it is experimental now. >> >> Signed-off-by: Wen Congyang >> Signed-off-by: zhanghailiang >> Signed-off-by: Gonglei >> Signed-off-by: Changlong Xie >> --- >> blockdev.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> qapi/block-core.json | 23 ++++++++++++++++++++++ >> qmp-commands.hx | 47 +++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 124 insertions(+) >> >> diff --git a/blockdev.c b/blockdev.c >> index 64dbfeb..4e62fdf 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -3836,6 +3836,60 @@ out: >> aio_context_release(aio_context); >> } >> >> +static BlockDriverState *bdrv_find_child(BlockDriverState *parent_bs, >> + const char *child_name) >> +{ >> + BdrvChild *child; >> + >> + QLIST_FOREACH(child, &parent_bs->children, next) { >> + if (strcmp(child->name, child_name) == 0) { >> + return child->bs; >> + } >> + } >> + >> + return NULL; >> +} >> + >> +void qmp_x_blockdev_change(const char *parent, bool has_child, >> + const char *child, bool has_node, >> + const char *node, Error **errp) >> +{ >> + BlockDriverState *parent_bs, *child_bs = NULL, *new_bs = NULL; >> + >> + parent_bs = bdrv_lookup_bs(parent, parent, errp); >> + if (!parent_bs) { >> + return; >> + } >> + >> + if (has_child == has_node) { >> + if (has_child) { >> + error_setg(errp, "The paramter child and node is conflict"); > > "The parameters child and node are in conflict" > > Or, more naturally: > > "child and node may not be specified at the same time" > OK >> + } else { >> + error_setg(errp, "Either child or node should be specified"); > > s/should/must/ Ok > >> + } >> + return; >> + } >> + >> + if (has_child) { >> + child_bs = bdrv_find_child(parent_bs, child); >> + if (!child_bs) { >> + error_setg(errp, "Node '%s' doesn't have child %s", > > s/doesn't/does not/ > > (This is a personal opinion, but a pretty strong one.) > > Also, if you put quotes around the node name, maybe you should do the > same around the child name. OK > >> + parent, child); >> + return; >> + } >> + bdrv_del_child(parent_bs, child_bs, errp); >> + } >> + >> + if (has_node) { >> + new_bs = bdrv_find_node(node); >> + if (!new_bs) { >> + error_setg(errp, "Node '%s' not found", node); >> + return; >> + } >> + bdrv_add_child(parent_bs, new_bs, errp); >> + } >> +} >> + >> BlockJobInfoList *qmp_query_block_jobs(Error **errp) >> { >> BlockJobInfoList *head = NULL, **p_next = &head; >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 1a5d9ce..fe63c6d 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -2408,3 +2408,26 @@ >> ## >> { 'command': 'block-set-write-threshold', >> 'data': { 'node-name': 'str', 'write-threshold': 'uint64' } } >> + >> +## >> +# @x-blockdev-change >> +# >> +# Dynamically reconfigure the block driver state graph. It can be used >> +# to add, remove, insert or replace a block driver state. Currently only > > I'd prefer "graph node" over BDS in this line. ok > >> +# the Quorum driver implements this feature to add or remove its child. >> +# This is useful to fix a broken quorum child. >> +# > > I'd like a list here what this command does depending on the parameters > given. For instance: > > If @(new-)node is specified, it will be inserted under @parent. @child > may not be specified in this case. > > If both @parent and @child are specified but @(new-)node is not, @child > will be detached from @parent. > ok >> +# @parent: the id or name of the node that will be changed. > > I don't know. The parent will actually not be changed, it's an edge that > will be changed; and the parent is the parent node of that edge. Just put > > @parent: the id or name of the parent node > > here. > Yes, you are right here. >> +# >> +# @child: #optional the name of the child that will be deleted. > > For now. But maybe that will change in the future. Generally, it is just > the child node of the edge that will be changed. So just putting > > @child: #optional the name of a child under the given parent node > > (Let's hope this is clear enough that people know that this is not a > node name.) > ok >> +# >> +# @node: #optional the name of the node will be added. > > Maybe this should be named new-node instead. > > Also: "...of the node that will be added." Yes, it's more clear. > >> +# >> +# Note: this command is experimental, and its API is not stable. >> +# >> +# Since: 2.6 >> +## >> +{ 'command': 'x-blockdev-change', >> + 'data' : { 'parent': 'str', >> + '*child': 'str', >> + '*node': 'str' } } >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index 7b235ee..efee0ca 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -4293,6 +4293,53 @@ Example: >> EQMP >> >> { >> + .name = "x-blockdev-change", >> + .args_type = "parent:B,child:B?,node:B?", >> + .mhandler.cmd_new = qmp_marshal_x_blockdev_change, >> + }, >> + >> +SQMP >> +x-blockdev-change >> +----------------- >> + >> +Dynamically reconfigure the block driver state graph. It can be used to >> +add, remove, insert, or replace a block driver state. Currently only >> +the Quorum driver implements this feature to add and remove its child. >> +This is useful to fix a broken quorum child. >> + >> +Arguments: >> +- "parent": the id or node name of which node will be changed (json-string) >> +- "child": the child name which will be deleted (json-string, optional) >> +- "node": the new node-name which will be added (json-string, optional) >> + >> +Note: this command is experimental, and not a stable API. It doesn't >> +support all kinds of operations, all kinds of children, nor all block >> +drivers. > > I think this second sentence should be present in qapi/block-core.json > as well. Will *highlight* it in qapi/block-core.json. Thanks for your review. Thanks -Xie > > Max > >> + >> +Example: >> + >> +Add a new node to a quorum >> +-> { "execute": "blockdev-add", >> + "arguments": { "options": { "driver": "raw", >> + "node-name": "new_node", >> + "id": "test_new_node", >> + "file": { "driver": "file", >> + "filename": "test.raw" } } } } >> +<- { "return": {} } >> +-> { "execute": "x-blockdev-change", >> + "arguments": { "parent": "disk1", >> + "node": "new_node" } } >> +<- { "return": {} } >> + >> +Delete a quorum's node >> +-> { "execute": "x-blockdev-change", >> + "arguments": { "parent": "disk1", >> + "child": "children.1" } } >> +<- { "return": {} } >> + >> +EQMP >> + >> + { >> .name = "query-named-block-nodes", >> .args_type = "", >> .mhandler.cmd_new = qmp_marshal_query_named_block_nodes, >> > >