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

[Stefan Hajnoczi]

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).

pgpv5arAD4J2N.pgp
Description: PGP signature