Re: [Qemu-devel] [PATCH v19 03/16] block: Introduce op_blockers to BlockDriverState

[Jeff Cody]

On Mon, May 19, 2014 at 04:37:52PM +0200, Kevin Wolf wrote:
> Am 19.05.2014 um 16:10 hat Markus Armbruster geschrieben:
> > Fam Zheng <address@hidden> writes:
> > 
> > > BlockDriverState.op_blockers is an array of lists with BLOCK_OP_TYPE_MAX
> > > elements. Each list is a list of blockers of an operation type
> > > (BlockOpType), that marks this BDS as currently blocked for a certain
> > > type of operation with reason errors stored in the list. The rule of
> > > usage is:
> > >
> > >  * BDS user who wants to take an operation should check if there's any
> > >    blocker of the type with bdrv_op_is_blocked().
> > >
> > >  * BDS user who wants to block certain types of operation, should call
> > >    bdrv_op_block (or bdrv_op_block_all to block all types of operations,
> > >    which is similar to the existing bdrv_set_in_use()).
> > >
> > >  * A blocker is only referenced by op_blockers, so the lifecycle is
> > >    managed by caller, and shouldn't be lost until unblock, so typically
> > >    a caller does these:
> > >
> > >    - Allocate a blocker with error_setg or similar, call bdrv_op_block()
> > >      to block some operations.
> > >    - Hold the blocker, do his job.
> > >    - Unblock operations that it blocked, with the same reason pointer
> > >      passed to bdrv_op_unblock().
> > >    - Release the blocker with error_free().
> > >
> > > Signed-off-by: Fam Zheng <address@hidden>
> > > Reviewed-by: Benoit Canet <address@hidden>
> > > Reviewed-by: Jeff Cody <address@hidden>
> > > ---
> > >  block.c                   | 75 
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/block/block.h     |  7 +++++
> > >  include/block/block_int.h |  5 ++++
> > >  3 files changed, 87 insertions(+)
> > >
> > > diff --git a/block.c b/block.c
> > > index b749d31..32338ca 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -335,6 +335,7 @@ void bdrv_register(BlockDriver *bdrv)
> > >  BlockDriverState *bdrv_new(const char *device_name, Error **errp)
> > >  {
> > >      BlockDriverState *bs;
> > > +    int i;
> > >  
> > >      if (bdrv_find(device_name)) {
> > >          error_setg(errp, "Device with id '%s' already exists",
> > > @@ -353,6 +354,9 @@ BlockDriverState *bdrv_new(const char *device_name, 
> > > Error **errp)
> > >      if (device_name[0] != '\0') {
> > >          QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
> > >      }
> > > +    for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
> > > +        QLIST_INIT(&bs->op_blockers[i]);
> > > +    }
> > >      bdrv_iostatus_disable(bs);
> > >      notifier_list_init(&bs->close_notifiers);
> > >      notifier_with_return_list_init(&bs->before_write_notifiers);
> > > @@ -1907,6 +1911,8 @@ static void 
> > > bdrv_move_feature_fields(BlockDriverState *bs_dest,
> > >      pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
> > >              bs_src->device_name);
> > >      bs_dest->device_list = bs_src->device_list;
> > > +    memcpy(bs_dest->op_blockers, bs_src->op_blockers,
> > > +           sizeof(bs_dest->op_blockers));
> > >  }
> > >  
> > >  /*
> > > @@ -5269,6 +5275,75 @@ void bdrv_unref(BlockDriverState *bs)
> > >      }
> > >  }
> > >  
> > > +struct BdrvOpBlocker {
> > > +    Error *reason;
> > > +    QLIST_ENTRY(BdrvOpBlocker) list;
> > > +};
> > > +
> > > +bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error 
> > > **errp)
> > > +{
> > > +    BdrvOpBlocker *blocker;
> > > +    assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
> > 
> > Space between cast and its operand is unusual.  Please don't respin just
> > for that.
> 
> That was a surprising statement for me. Do you have an idea how to grep
> for casts? I tried '*)' just in order to find _some_ examples of casts,
> and there doesn't seem to be a clear winner. But if there is one, it
> appears to be the version with space.
> 
> (I won't reject patches with either style.)
>

This just searches for pointer casts - it isn't perfect, but pretty
decent if you just want to get a sampling:

grep -E "\([a-zA-Z0-9_]+[\\*\ ]+\)" * -rHnI