From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58766) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWeUe-0000T4-0Z for qemu-devel@nongnu.org; Tue, 01 Sep 2015 01:52:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWeUc-0005hf-VM for qemu-devel@nongnu.org; Tue, 01 Sep 2015 01:52:27 -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: <55E53CF2.1030605@cn.fujitsu.com> Date: Tue, 1 Sep 2015 13:51:46 +0800 MIME-Version: 1.0 In-Reply-To: <55E4A536.6040905@redhat.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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(+) >> >=20 >> +void qmp_child_add(const char *device, BlockdevOptionsChild *options, >> + Error **errp) >> +{ >> + QmpOutputVisitor *ov =3D qmp_output_visitor_new(); >> + QObject *obj; >> + QDict *qdict; >> + Error *local_err =3D 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_zer= oes) { >> + error_setg(errp, "id, discard, cache, aio, rerror, werror, read= only" >> + " and detect_zeroes cann't be used for child-add"); >=20 > s/cann't/can't/ >=20 > 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? >=20 >> +++ 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' } } >=20 > Do you need this struct? It causes extra nesting on the wire... >=20 >> + >> +## >> +# @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' } } >=20 > ...Consider if you just did: >=20 > { 'command': 'child-add', > 'data': { 'device', 'str', 'child': 'BlockdevOptions' } } >=20 >> >> + >> +## >> +# @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. >=20 > Might also be worth mentioning that you can't remove a child if it would > bring the quorum below its threshold. >=20 >> +++ b/qmp-commands.hx >=20 >> +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. >=20 > Maybe we should name it 'x-child-add' for now, so that we aren't baking > ourselves into a corner. >=20 >> + >> +Arguments: >> + >> +- "device": the quorum's id or node name >> +- "options": the new child options >> + >> +Example: >> + >> +-> { "execute": "child-add", >> + "arguments": { >> + "device": "disk1", >> + "options" : { >> + "child": { >=20 > ...the simper command idea above would reduce one layer of {} nesting her= e. >=20 When we open a child BDS, the options for child BDS must have the same pref= ix, such as file.xxx, x-image.xxx, ... For hot-added child BDS, the prefix is "child=E2= =80=9C. If we don't use "options" here, the prefix "child" will be eaten in qmp_marshal_input_x= _child_add(). I am investigating how to solve it. Any suggestion is welcome. Thanks Wen Congyang