On 17.05.2016 09:35, Fam Zheng wrote: > Stash the locking state into BDRVReopenState. If it was locked, unlock > in prepare, and lock it again when commit or abort. > > Signed-off-by: Fam Zheng <[email protected]> > --- > block.c | 11 +++++++++++ > include/block/block.h | 1 + > 2 files changed, 12 insertions(+) > > diff --git a/block.c b/block.c > index ad3663c..2be42bb 100644 > --- a/block.c > +++ b/block.c > @@ -2112,6 +2112,11 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, > BlockReopenQueue *queue, > } while ((entry = qdict_next(reopen_state->options, entry))); > } > > + reopen_state->was_locked = reopen_state->bs->image_locked; > + if (reopen_state->was_locked) { > + bdrv_unlock_image(reopen_state->bs); > + } > + > ret = 0; > > error: > @@ -2136,6 +2141,9 @@ static void bdrv_reopen_commit(BDRVReopenState > *reopen_state) > if (drv->bdrv_reopen_commit) { > drv->bdrv_reopen_commit(reopen_state); > } > + if (reopen_state->was_locked) { > + bdrv_lock_image(reopen_state->bs); > + } > > /* set BDS specific flags now */ > QDECREF(reopen_state->bs->explicit_options); > @@ -2162,6 +2170,9 @@ static void bdrv_reopen_abort(BDRVReopenState > *reopen_state) > if (drv->bdrv_reopen_abort) { > drv->bdrv_reopen_abort(reopen_state); > } > + if (reopen_state->was_locked) { > + bdrv_lock_image(reopen_state->bs);
I'm not sure I'm quite happy with this, because this may fail;
therefore, it may happen that the operation done in _prepare() cannot be
undone.
Of course, the same applies to _commit(): bdrv_lock_image() can just
fail. It's probably even worse there. I don't see a good reason why just
relocking the image in _abort() should fail; but it's very much
conceivable that locking the reopened image in _commit() fails.
I think the correct way would be to rely on the drivers which implement
locking to handle reopening correctly, that is, try lock the reopened
image in _prepare() and unlock the old one before discarding it in
_commit().
However, I'm not sure myself whether it's worth the effort. It's very
simple to do it like you did here -- but then again, it doesn't seem
quite correct.
Also: Should bdrv_reopen_prepare() check that the locking flags are not
changed?
Max
> + }
>
> QDECREF(reopen_state->explicit_options);
> }
> diff --git a/include/block/block.h b/include/block/block.h
> index 7740d3f..28b8ae9 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -158,6 +158,7 @@ typedef struct BDRVReopenState {
> QDict *options;
> QDict *explicit_options;
> void *opaque;
> + bool was_locked;
> } BDRVReopenState;
>
> /*
>
signature.asc
Description: OpenPGP digital signature
