Re: [Qemu-devel] [Patch for-2.5 v2 5/6] qmp: add monitor command to add/remove a child

[Eric Blake]

On 08/11/2015 01:51 AM, Wen Congyang wrote:
> Signed-off-by: Wen Congyang <address@hidden>
> Signed-off-by: zhanghailiang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> ---
>  blockdev.c           | 79 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 40 ++++++++++++++++++++++++++
>  qmp-commands.hx      | 67 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 186 insertions(+)
> 

> +void qmp_child_add(const char *device, BlockdevOptionsChild *options,
> +                   Error **errp)
> +{
> +    QmpOutputVisitor *ov = qmp_output_visitor_new();
> +    QObject *obj;
> +    QDict *qdict;
> +    Error *local_err = NULL;
> +
> +    if (options->child->has_id || options->child->has_discard ||
> +        options->child->has_cache || options->child->has_aio ||
> +        options->child->has_rerror || options->child->has_werror ||
> +        options->child->has_read_only || options->child->has_detect_zeroes) {
> +        error_setg(errp, "id, discard, cache, aio, rerror, werror, readonly"
> +                   " and detect_zeroes cann't be used for child-add");

s/cann't/can't/

If they can't be used, then why not write the qapi so that they can't
even be provided?  On the other hand, why can't they be used?  Can't you
specify some of these options separately for different quorum children
when first creating a quorum, in which case you'd want to be able to do
likewise when adding a new member to the quorum?

> +++ b/qapi/block-core.json
> @@ -2122,3 +2122,43 @@
>  ##
>  { 'command': 'block-set-write-threshold',
>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
> +
> +##
> +# @BlockdevOptionsChild
> +#
> +# Driver specific block device options for child block device.
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'BlockdevOptionsChild',
> +  'data': { 'child': 'BlockdevOptions' } }

Do you need this struct?  It causes extra nesting on the wire...

> +
> +##
> +# @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.
> +#
> +# @device: graph node name or id which the child will be added to.
> +#
> +# @options: the options to create child BDS.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'child-add',
> +  'data' : { 'device': 'str', 'options': 'BlockdevOptionsChild' } }

...Consider if you just did:

{ 'command': 'child-add',
  'data': { 'device', 'str', 'child': 'BlockdevOptions' } }

> 
> +
> +##
> +# @child-del
> +#
> +# Remove a child from the parent BDS. Currently only the Quorum driver
> +# implements this feature. This is useful to fix a broken quorum child.

Might also be worth mentioning that you can't remove a child if it would
bring the quorum below its threshold.

> +++ b/qmp-commands.hx

> +Add a child to a quorum node.
> +
> +This command is still a work in progress. It doesn't support all
> +block drivers. Stay away from it unless you want it to help with
> +its development.

Maybe we should name it 'x-child-add' for now, so that we aren't baking
ourselves into a corner.

> +
> +Arguments:
> +
> +- "device": the quorum's id or node name
> +- "options": the new child options
> +
> +Example:
> +
> +-> { "execute": "child-add",
> +    "arguments": {
> +        "device": "disk1",
> +        "options" : {
> +            "child": {

...the simper command idea above would reduce one layer of {} nesting here.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature