Re: [Qemu-block] [PATCH v6 3/4] qmp: add monitor command to add/remove a child

[Wen Congyang]

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
> .
>