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 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?

We have decieded use this way here:
http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02975.html

Thanks
Wen Congyang

> 
> That is cleaner because it doesn't introduce a new state (which is only
> implemented for nbd).
>