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

[Wen Congyang]

On 09/01/2015 03:04 AM, Eric Blake wrote:
> 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.
> 

When we open a child BDS, the options for child BDS must have the same prefix, 
such as
file.xxx, x-image.xxx, ... For hot-added child BDS, the prefix is "child“. If 
we don't
use "options" here, the prefix "child" will be eaten in 
qmp_marshal_input_x_child_add().
I am investigating how to solve it. Any suggestion is welcome.

Thanks
Wen Congyang