Re: [Qemu-block] [PATCH v10 1/3] Add new block driver interface to add/delete a BDS's child

[Max Reitz]

On 07.03.2016 05:16, Changlong Xie wrote:
> On 03/06/2016 01:27 AM, Max Reitz wrote:
>> Sorry that I wasn't so pedantic last time; or maybe I should rather be
>> sorry that I'm so pedantic this time.
> 
> Hi Max
>     Welcome all your comments : )

Good :-)

>> On 16.02.2016 10:37, Changlong Xie wrote:
>>> From: Wen Congyang <address@hidden>
>>>
>>> In some cases, we want to take a quorum child offline, and take
>>> another child online.
>>>
>>> Signed-off-by: Wen Congyang <address@hidden>
>>> Signed-off-by: zhanghailiang <address@hidden>
>>> Signed-off-by: Gonglei <address@hidden>
>>> Signed-off-by: Changlong Xie <address@hidden>
>>> Reviewed-by: Eric Blake <address@hidden>
>>> Reviewed-by: Alberto Garcia <address@hidden>
>>> ---
>>>   block.c                   | 50
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/block/block.h     |  5 +++++
>>>   include/block/block_int.h |  5 +++++
>>>   3 files changed, 60 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index efc3c43..08aa979 100644
>>> --- a/block.c
>>> +++ b/block.c

[...]

>>> +        return;
>>> +    }
>>> +
>>> +    QLIST_FOREACH(child, &parent_bs->children, next) {
>>> +        if (child->bs == child_bs) {
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    if (!child) {
>>> +        error_setg(errp, "The node %s is not a child of %s",
>>> +                   bdrv_get_device_or_node_name(child_bs),
>>> +                   bdrv_get_device_or_node_name(parent_bs));
>>
>> Is there a special reason why you are using
>> bdrv_get_device_or_node_name() for the child here, while in
>> bdrv_add_child() you directly use the node name?
>>
> 
> Although we directly use the node name in bdrv_add_child(), but we still
> need treat bdrv_del_child() as a separate function, right? In up
> condition, we can't determine if child->bs supports BB or not, so i
> think bdrv_get_device_or_node_name(child->bs) is ok here.

I just realized that in order to invoke bdrv_add_child() one passes a
node name to x-blockdev-change, whereas for bdrv_del_child() name of the
child is passed (which is not a node name).

So it makes perfect sense to always emit the node name in
bdrv_add_child(), regardless of whether the BDS has a BB, because the
node name was the parameter that had been given to x-blockdev-change.

In contrast, the supposedly child node passed to bdrv_del_child() is not
identified by its node name, so it makes sense not to limit the output
to the node name but to print the BB's name if present.

So indeed, this is completely fine as it is in this patch.

Max

>> Neither seems wrong to me. A child is unlikely to have a BB of its own,
>> but if it does, printing its name instead of the node name may be
> 
> bdrv_get_device_or_node_name() can do that.
> 
> Thanks
>     -Xie
> 
>> appropriate. I'm just wondering about the difference between
>> bdrv_add_child() and bdrv_del_child().
>>
>> Max
>>
>>> +        return;
>>> +    }
>>> +
>>> +    parent_bs->drv->bdrv_del_child(parent_bs, child_bs, errp);
>>> +}
>>> diff --git a/include/block/block.h b/include/block/block.h
>>> index 1c4f4d8..ecde190 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -582,4 +582,9 @@ void bdrv_drained_begin(BlockDriverState *bs);
>>>    */
>>>   void bdrv_drained_end(BlockDriverState *bs);
>>>
>>> +void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
>>> +                    Error **errp);
>>> +void bdrv_del_child(BlockDriverState *parent, BlockDriverState *child,
>>> +                    Error **errp);
>>> +
>>>   #endif
>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>> index 9ef823a..89ec138 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -305,6 +305,11 @@ struct BlockDriver {
>>>        */
>>>       void (*bdrv_drain)(BlockDriverState *bs);
>>>
>>> +    void (*bdrv_add_child)(BlockDriverState *parent,
>>> BlockDriverState *child,
>>> +                           Error **errp);
>>> +    void (*bdrv_del_child)(BlockDriverState *parent,
>>> BlockDriverState *child,
>>> +                           Error **errp);
>>> +
>>>       QLIST_ENTRY(BlockDriver) list;
>>>   };
>>>
>>>
>>
>>
> 
>

signature.asc
Description: OpenPGP digital signature