On 11/09/2015 10:42 PM, Alberto Garcia wrote:
> 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'.
OK, I will fix it in the next version
>
>> + 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.
Too many codes in qemu use local_err and error_propagate(). I think
errp can be NOT NULL here(in which case?).
>
>> +
>> + 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.
I forgot to do it...
>
>> + 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.
OK
>
>> +##
>> +# @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".
Thanks for your review, all will be fixed in the next version
Wen Congyang
>
> Berto
> .
>
