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

[Markus Armbruster]

Wen Congyang <address@hidden> writes:

> On 09/15/2015 03:49 PM, Markus Armbruster wrote:
>> Wen Congyang <address@hidden> writes:
>> 
>>> On 09/14/2015 10:36 PM, Markus Armbruster wrote:
>>>> Wen Congyang <address@hidden> writes:
>>>>
>>>>> Signed-off-by: Wen Congyang <address@hidden>
>>>>> Signed-off-by: zhanghailiang <address@hidden>
>>>>> Signed-off-by: Gonglei <address@hidden>
>>>>> ---
>>>>>  blockdev.c           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>>>>>  qmp-commands.hx      | 53 
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 134 insertions(+)
>>>>>
>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>> index bd47756..0a40607 100644
>>>>> --- a/blockdev.c
>>>>> +++ b/blockdev.c
>>>>> @@ -3413,6 +3413,53 @@ fail:
>>>>>      qmp_output_visitor_cleanup(ov);
>>>>>  }
>>>>>  
>>>>> +void qmp_x_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;
>>>>> +    }
>>>>> +
>>>>> +    child_bs = bdrv_find_node(child);
>>>>> +    if (!child_bs) {
>>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    bdrv_add_child(parent_bs, child_bs, &local_err);
>>>>> +    if (local_err) {
>>>>> +        error_propagate(errp, local_err);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +void qmp_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;
>>>>> +    }
>>>>> +
>>>>> +    child_bs = bdrv_find_node(child);
>>>>> +    if (!child_bs) {
>>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    bdrv_del_child(parent_bs, child_bs, &local_err);
>>>>> +    if (local_err) {
>>>>> +        error_propagate(errp, local_err);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>  BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>>>>>  {
>>>>>      BlockJobInfoList *head = NULL, **p_next = &head;
>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>>> index e68a59f..b959577 100644
>>>>> --- a/qapi/block-core.json
>>>>> +++ b/qapi/block-core.json
>>>>> @@ -2272,3 +2272,37 @@
>>>>>  ##
>>>>>  { 'command': 'block-set-write-threshold',
>>>>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>>>>> +
>>>>> +##
>>>>> +# @x-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.
>>>>> +#
>>>>> +# @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.
>>>>> +#
>>>>> +# Since: 2.5
>>>>> +##
>>>>> +{ 'command': 'x-child-add',
>>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>>> +
>>>>> +##
>>>>> +# @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.
>>>>> +# Note, you can't remove a child if it would bring the quorum below its
>>>>> +# threshold.
>>>>> +#
>>>>> +# @parent: graph node name or id from which the child will removed.
>>>>> +#
>>>>> +# @child: graph node name that will be removed.
>>>>> +#
>>>>> +# Since: 2.5
>>>>> +##
>>>>> +{ 'command': 'child-del',
>>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>>
>>>> Why is x-child-add experimental, but child-del isn't?  Please explain
>>>> both in the schema and in the commit message.
>>>
>>> No special reason. Should I put child-del in experimental namespace?
>> 
>> I found the reason for x-child-add in your v2:
>> 
>>     child-add
>>     ------------
>> 
>>     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.
>> 
>> Eric suggested to rename it to x-child-add, and you did.  Good.  You
>> also shortened the "work in progress" note to just "Note: this command
>> is experimental, and not a stable API."  I'd like to have a more verbose
>> note explaining *why* the command is experimental, both here and in
>> qmp-commands.hx.  "It doesn't support all block drivers" is a reason.
>> Are the any others?
>> 
>> Is child-del similarly unfinished?  If yes, make it x-child-del to save
>> us from later grief.
>> 
>> If no: is child-del is only useful together with x-child-add?  Then make
>> it x-child-del regardless.
>
> I have another question: if the command is experimental, we have the
> prefix "x-".
> Which prefix is used for hmp command?

HMP is not a stable interface, so generally don't bother marking
experimental interfaces.

That said, I'd probably keep HMP and QMP command name the same to
minimize confusion.

[...]