[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v19 03/16] block: Introduce op_blockers to Block
From: |
Jeff Cody |
Subject: |
Re: [Qemu-devel] [PATCH v19 03/16] block: Introduce op_blockers to BlockDriverState |
Date: |
Mon, 19 May 2014 11:37:52 -0400 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
[Qemu-devel] [PATCH v19 05/16] block: Move op_blocker check from block_job_create to its caller, Fam Zheng, 2014/05/11
[Qemu-devel] [PATCH v19 04/16] block: Replace in_use with operation blocker, Fam Zheng, 2014/05/11
[Qemu-devel] [PATCH v19 06/16] block: Add bdrv_set_backing_hd(), Fam Zheng, 2014/05/11
[Qemu-devel] [PATCH v19 07/16] block: Add backing_blocker in BlockDriverState, Fam Zheng, 2014/05/11