Wen Congyang <we...@cn.fujitsu.com> writes:
> On 09/15/2015 03:49 PM, Markus Armbruster wrote:
>> Wen Congyang <we...@cn.fujitsu.com> writes:
>>
>>> On 09/14/2015 10:36 PM, Markus Armbruster wrote:
>>>> Wen Congyang <we...@cn.fujitsu.com> writes:
>>>>
>>>>> Signed-off-by: Wen Congyang <we...@cn.fujitsu.com>
>>>>> Signed-off-by: zhanghailiang <zhang.zhanghaili...@huawei.com>
>>>>> Signed-off-by: Gonglei <arei.gong...@huawei.com>
>>>>> ---
>>>>> 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.
[...]
