qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alberto Garcia
Subject: Re: [Qemu-devel] [PATCH v6 3/4] qmp: add monitor command to add/remove a child
Date: Mon, 09 Nov 2015 15:42:59 +0100
User-agent: Notmuch/0.13.2 (http://notmuchmail.org) Emacs/23.2.1 (i486-pc-linux-gnu)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]