Re: [Qemu-devel] [Qemu-block] [PATCH COLO-Block v6 07/16] Add new block driver interface to connect/disconnect the remote target

[Wen Congyang]

On 06/19/2015 06:49 PM, Stefan Hajnoczi wrote:
> On Fri, Jun 19, 2015 at 08:54:56AM +0800, Wen Congyang wrote:
>> On 06/19/2015 12:06 AM, Stefan Hajnoczi wrote:
>>> On Thu, Jun 18, 2015 at 10:36:39PM +0800, Wen Congyang wrote:
>>>> At 2015/6/18 20:55, Stefan Hajnoczi Wrote:
>>>>> On Thu, Jun 18, 2015 at 04:49:12PM +0800, Wen Congyang wrote:
>>>>>> +void bdrv_connect(BlockDriverState *bs, Error **errp)
>>>>>> +{
>>>>>> +    BlockDriver *drv = bs->drv;
>>>>>> +
>>>>>> +    if (drv && drv->bdrv_connect) {
>>>>>> +        drv->bdrv_connect(bs, errp);
>>>>>> +    } else if (bs->file) {
>>>>>> +        bdrv_connect(bs->file, errp);
>>>>>> +    } else {
>>>>>> +        error_setg(errp, "this feature or command is not currently 
>>>>>> supported");
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +void bdrv_disconnect(BlockDriverState *bs)
>>>>>> +{
>>>>>> +    BlockDriver *drv = bs->drv;
>>>>>> +
>>>>>> +    if (drv && drv->bdrv_disconnect) {
>>>>>> +        drv->bdrv_disconnect(bs);
>>>>>> +    } else if (bs->file) {
>>>>>> +        bdrv_disconnect(bs->file);
>>>>>> +    }
>>>>>> +}
>>>>>
>>>>> Please add doc comments describing the semantics of these commands.
>>>>
>>>> Where should it be documented? In the header file?
>>>
>>> block.h doesn't document prototypes in the header file, please document
>>> the function definition in block.c.  (QEMU is not consistent here, some
>>> places do it the other way around.)
>>>
>>>>> Why are these operations needed when there is already a bs->drv == NULL
>>>>> case which means the BDS is not ready for read/write?
>>>>>
>>>>
>>>> The purpos is that: don't connect to nbd server when opening a nbd client.
>>>> connect/disconnect
>>>> to nbd server when we need to do it.
>>>>
>>>> IIUC, if bs->drv is NULL, it means that the driver is ejected? Here,
>>>> connect/disconnect
>>>> means that connect/disconnect to remote target(The target may be in another
>>>> host).
>>>
>>> Connect/disconnect puts something on the QEMU command-line that isn't
>>> ready at startup time.
>>>
>>> How about using monitor commands to add objects when needed instead?
>>>
>>> That is cleaner because it doesn't introduce a new state (which is only
>>> implemented for nbd).
>>>
>>
>> The problem is that, nbd client is one child of quorum, and quorum must have 
>> more
>> than one child. The nbd server is not ready until colo is running.
> 
> A monitor command to hot add/remove quorum children solves this problem
> and could also be used in other scenarios (e.g. user decides to take a
> quorum child offline).
> 


CCing Alberto Garcia for the quorum block driver.