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

[Kevin Wolf]

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

Kevin