From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55948) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWZrT-0006Uu-Jb for qemu-devel@nongnu.org; Mon, 31 Aug 2015 20:55:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWZrS-0001Cu-H3 for qemu-devel@nongnu.org; Mon, 31 Aug 2015 20:55:43 -0400 References: <1439279489-13338-1-git-send-email-wency@cn.fujitsu.com> <1439279489-13338-6-git-send-email-wency@cn.fujitsu.com> <55E4A536.6040905@redhat.com> From: Wen Congyang Message-ID: <55E4F767.1070602@cn.fujitsu.com> Date: Tue, 1 Sep 2015 08:55:03 +0800 MIME-Version: 1.0 In-Reply-To: <55E4A536.6040905@redhat.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Patch for-2.5 v2 5/6] qmp: add monitor command to add/remove a child List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu devel , Markus Armbruster , Alberto Garcia , Stefan Hajnoczi Cc: Kevin Wolf , zhanghailiang , qemu block , Jiang Yunhong , Dong Eddie , "Dr. David Alan Gilbert" , Gonglei , Yang Hongyang On 09/01/2015 03:04 AM, Eric Blake wrote: > On 08/11/2015 01:51 AM, Wen Congyang wrote: >> Signed-off-by: Wen Congyang >> Signed-off-by: zhanghailiang >> Signed-off-by: Gonglei >> --- >> blockdev.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> qapi/block-core.json | 40 ++++++++++++++++++++++++++ >> qmp-commands.hx | 67 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 186 insertions(+) >> > >> +void qmp_child_add(const char *device, BlockdevOptionsChild *options, >> + Error **errp) >> +{ >> + QmpOutputVisitor *ov = qmp_output_visitor_new(); >> + QObject *obj; >> + QDict *qdict; >> + Error *local_err = NULL; >> + >> + if (options->child->has_id || options->child->has_discard || >> + options->child->has_cache || options->child->has_aio || >> + options->child->has_rerror || options->child->has_werror || >> + options->child->has_read_only || options->child->has_detect_zeroes) { >> + error_setg(errp, "id, discard, cache, aio, rerror, werror, readonly" >> + " and detect_zeroes cann't be used for child-add"); > > s/cann't/can't/ > > If they can't be used, then why not write the qapi so that they can't > even be provided? On the other hand, why can't they be used? Can't you > specify some of these options separately for different quorum children > when first creating a quorum, in which case you'd want to be able to do > likewise when adding a new member to the quorum? IIUC, it is just top BDS's option, and it is not be used by the hot-added child. The hot-added child will use its parent flags. > >> +++ b/qapi/block-core.json >> @@ -2122,3 +2122,43 @@ >> ## >> { 'command': 'block-set-write-threshold', >> 'data': { 'node-name': 'str', 'write-threshold': 'uint64' } } >> + >> +## >> +# @BlockdevOptionsChild >> +# >> +# Driver specific block device options for child block device. >> +# >> +# Since: 2.5 >> +## >> +{ 'struct': 'BlockdevOptionsChild', >> + 'data': { 'child': 'BlockdevOptions' } } > > Do you need this struct? It causes extra nesting on the wire... > >> + >> +## >> +# @child-add >> +# >> +# Add a new child to the parent BDS. Currently only the Quorum driver >> +# implements this feature. This is useful to fix a broken quorum child. >> +# >> +# @device: graph node name or id which the child will be added to. >> +# >> +# @options: the options to create child BDS. >> +# >> +# Since: 2.5 >> +## >> +{ 'command': 'child-add', >> + 'data' : { 'device': 'str', 'options': 'BlockdevOptionsChild' } } > > ...Consider if you just did: > > { 'command': 'child-add', > 'data': { 'device', 'str', 'child': 'BlockdevOptions' } } OK, I will try it. > >> >> + >> +## >> +# @child-del >> +# >> +# Remove a child from the parent BDS. Currently only the Quorum driver >> +# implements this feature. This is useful to fix a broken quorum child. > > Might also be worth mentioning that you can't remove a child if it would > bring the quorum below its threshold. > >> +++ b/qmp-commands.hx > >> +Add a child to a quorum node. >> + >> +This command is still a work in progress. It doesn't support all >> +block drivers. Stay away from it unless you want it to help with >> +its development. > > Maybe we should name it 'x-child-add' for now, so that we aren't baking > ourselves into a corner. Do you mean the command name should be x-child-add? It is OK. > >> + >> +Arguments: >> + >> +- "device": the quorum's id or node name >> +- "options": the new child options >> + >> +Example: >> + >> +-> { "execute": "child-add", >> + "arguments": { >> + "device": "disk1", >> + "options" : { >> + "child": { > > ...the simper command idea above would reduce one layer of {} nesting here. > Yes, I will try it. Thanks Wen Congyang