Re: [Qemu-devel] [PATCH 2/8] block: add driver operation callbacks

[Liu Yuan]

On Mon, Sep 01, 2014 at 11:28:22AM +0200, Benoît Canet wrote:
> The Monday 01 Sep 2014 à 17:19:19 (+0800), Liu Yuan wrote :
> > On Mon, Sep 01, 2014 at 10:28:54AM +0200, Benoît Canet wrote:
> > > The Monday 01 Sep 2014 à 15:43:08 (+0800), Liu Yuan wrote :
> > > > Driver operations are defined as callbacks passed from block upper 
> > > > drivers to
> > > > lower drivers and are supposed to be called by lower drivers.
> > > > 
> > > > Requests handling(queuing, submitting, etc.) are done in protocol tier 
> > > > in the
> > > > block layer and connection states are also maintained down there. Driver
> > > > operations are supposed to notify the upper tier (such as quorum) of 
> > > > the states
> > > > changes.
> > > > 
> > > > For now only two operation are added:
> > > > 
> > > > driver_disconnect: called when connection is off
> > > > driver_reconnect: called when connection is on after disconnection
> > > > 
> > > > Which are used to notify upper tier of the connection state.
> > > > 
> > > > Cc: Eric Blake <address@hidden>
> > > > Cc: Benoit Canet <address@hidden>
> > > > Cc: Kevin Wolf <address@hidden>
> > > > Cc: Stefan Hajnoczi <address@hidden>
> > > > Signed-off-by: Liu Yuan <address@hidden>
> > > > ---
> > > >  block.c                   | 7 +++++++
> > > >  include/block/block.h     | 7 +++++++
> > > >  include/block/block_int.h | 3 +++
> > > >  3 files changed, 17 insertions(+)
> > > > 
> > > > diff --git a/block.c b/block.c
> > > > index c12b8de..22eb3e4 100644
> > > > --- a/block.c
> > > > +++ b/block.c
> > > > @@ -2152,6 +2152,13 @@ void bdrv_set_dev_ops(BlockDriverState *bs, 
> > > > const BlockDevOps *ops,
> > > >      bs->dev_opaque = opaque;
> > > >  }
> > > >  
> > > > +void bdrv_set_drv_ops(BlockDriverState *bs, const BlockDrvOps *ops,
> > > > +                      void *opaque)
> > > > +{
> > > > +    bs->drv_ops = ops;
> > > > +    bs->drv_opaque = opaque;
> > > 
> > > We need to be very carefull of the mix between these fields and the 
> > > infamous
> > > bdrv_swap function.
> > > 
> > > Also I don't know if "driver operations" is the right name since the 
> > > BlockDriver structure's
> > > callback could also be named "driver operations".
> > > 
> > 
> > BlockDrvierState has a "device operation" for callbacks from devices. So I
> > choose "driver operation". So any sugguestion for better name?
> 
> From what I see in this series the job of these callbacks is to send a 
> message or a signal to
> the upper BDS.
> 
> Also the name must reflect it goes from the child to the parent.
> 
> child_signals ?
> child_messages ?
> 

As far as I see, put child in the name will make it too quorum centric. Since it
is operation in BlockDriverState, we need to keep it as generic as we could.

These operations [here we mean disconnect() and reconnect(), but probably later
some other will add more opeartions] are passed from 'upper driver' to protocol
driver [in the code we call the protocol as 'file' driver, a narrow name too],
so I chose to name it as 'driver operation'. If we can rename 'file' as
protocol, include file, nfs, sheepdog, etc, such as

bdrv_create_file -> bdrv_create_protocol
bs.file -> bs.protocol

then the 'driver operation' here would sound better.

Thanks
Yuan