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

[Alberto Garcia]

On Tue 22 Sep 2015 09:44:21 AM CEST, Wen Congyang wrote:
> The new QMP command name is x-blockdev-child-add, and x-blockdev-child-del.
> It justs for adding/removing quorum's child now, and don't support all
> kinds of children, nor all block drivers. So it is experimental now.

Better "So it is experimental for now". Otherwise it suggests that it
was not experimental before but now it is.

> +void qmp_x_blockdev_child_add(const char *parent, const char *child,
> +                              Error **errp)
> +{
> +    BlockDriverState *parent_bs, *child_bs;
> +    Error *local_err = NULL;
> +
> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
> +    if (!parent_bs) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }

I think this is fine as it is now, but since you are checking the value
of parent_bs to see if bdrv_lookup_bs() succeeded you can use errp
directly instead and skip the error_propagate() call.

> +void qmp_x_blockdev_child_del(const char *parent, const char *child,
> +                              Error **errp)
> +{
> +    BlockDriverState *parent_bs, *child_bs;
> +    Error *local_err = NULL;
> +
> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
> +    if (!parent_bs) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }

Same here.

> +##
> +# @x-blockdev-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.

Again, I would not use 'BDS' here. "Add a new child to an existing block
device", or something like that.

> +# @parent: graph node name or id which the child will be added to.
> +#
> +# @child: graph node name that will be added.
> +#
> +# Note: this command is experimental, and not a stable API.

"[..] and does not have a stable API".

> +x-blockdev-child-add
> +------------
> +
> +Add a child to a quorum node.
> +
> +Arguments:
> +
> +- "parent": the quorum's id or node name
> +- "child": the child node-name which will be added
> +

I would use the same kind of description than in the json file. "Add a
child to an existing block device. Currently only Quorum supports this
feature". Same for the 'x-blockdev-child-del' documentation.

Berto