From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40687) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zvnf0-000102-SW for qemu-devel@nongnu.org; Mon, 09 Nov 2015 09:43:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zvnew-0002v7-Jl for qemu-devel@nongnu.org; Mon, 09 Nov 2015 09:43:06 -0500 From: Alberto Garcia In-Reply-To: <1444985866-12969-4-git-send-email-wency@cn.fujitsu.com> References: <1444985866-12969-1-git-send-email-wency@cn.fujitsu.com> <1444985866-12969-4-git-send-email-wency@cn.fujitsu.com> Date: Mon, 09 Nov 2015 15:42:59 +0100 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH v6 3/4] qmp: add monitor command to add/remove a child List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wen Congyang , qemu devel , Eric Blake , Markus Armbruster , Kevin Wolf , Stefan Hajnoczi Cc: qemu block , Jiang Yunhong , Dong Eddie , "Dr. David Alan Gilbert" , Gonglei , Yang Hongyang , zhanghailiang Sorry again for the late review, here are my comments: On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang wrote: > +void qmp_x_blockdev_change(ChangeOperation op, const char *parent, > + bool has_child, const char *child, > + bool has_new_node, const char *new_node, > + Error **errp) You are using different names for the parameters here: 'op', 'parent', 'child', 'new_node'; in the JSON file the first and last one are named 'operation' and 'node'. > + parent_bs = bdrv_lookup_bs(parent, parent, &local_err); > + if (!parent_bs) { > + error_propagate(errp, local_err); > + return; > + } You don't need to change it if you don't want but you can use errp directly here and spare the error_propagate() call. > + > + switch(op) { > + case CHANGE_OPERATION_ADD: > + if (has_child) { > + error_setg(errp, "The operation %s doesn't support the parameter child", > + ChangeOperation_lookup[op]); > + return; > + } This line goes over 80 columns, please use scripts/checkpatch.pl to check the style of the code. > + if (!has_new_node) { > + error_setg(errp, "The operation %s needs the parameter new_node", > + ChangeOperation_lookup[op]); > + return; > + } > + break; > + case CHANGE_OPERATION_DELETE: > + if (has_new_node) { > + error_setg(errp, "The operation %s doesn't support the parameter node", > + ChangeOperation_lookup[op]); > + return; > + } > + if (!has_child) { > + error_setg(errp, "The operation %s needs the parameter child", > + ChangeOperation_lookup[op]); > + return; > + } I still think that having two optional parameters here makes things unnecessarily complex. If in the future we want to add a new operation that needs a new parameter then we can add it then, but I think that both 'add' and 'delete' can work perfectly fine with a single 'node' parameter. Does anyone else have an opinion about this? > + default: > + break; > + } This is unreachable so you can add a g_assert_not_reached() here. > +## > +# @ChangeOperation: > +# > +# An enumeration of block device change operation. How about something like "An enumeration of possible change operations on a block device" ? > +# @add: Add a new block driver state to a existed block driver state. s/a existed/an existing/ > +# @x-blockdev-change > +# > +# Dynamic reconfigure the block driver state graph. It can be used to "Dynamically reconfigure" > +# add, remove, insert, replace a block driver state. Currently only "insert or replace" > +# the Quorum driver implements this feature to add and remove its child. > +# This is useful to fix a broken quorum child. "add and remove its child" -> "add or remove a child" > +# > +# @operation: the chanage operation. It can be add, delete. s/chanage/change/ > +# > +# @parent: the id or node name of which node will be changed. How about "the id or name of the node that will be changed" ? > +# > +# @child: the child node-name which will be deleted. > +# > +# @node: the new node-name which will be added. "The name of the node that will be deleted" "The name of the node that will be added" Or, if you merge both parameters, "...that will be added or deleted". > +# > +# Note: this command is experimental, and not a stable API. "and not a stable API" -> "and does not have a stable API", or "and its API is not stable". Berto