On Fri 16 Nov 2018 04:53:00 PM CET, Max Reitz wrote: > bdrv_reopen_multiple() does not invoke bdrv_reopen_abort() for the > element of the reopen queue for which bdrv_reopen_prepare() failed, > because it assumes that the prepare function will have rolled back all > changes already. > > However, bdrv_reopen_prepare() does not do this in every case: It may > notice an error after BlockDriver.bdrv_reopen_prepare() succeeded, and > it will not invoke BlockDriver.bdrv_reopen_abort() then; and neither > will bdrv_reopen_multiple(), as explained above. > > This is wrong because we must always call .bdrv_reopen_commit() or > .bdrv_reopen_abort() after .bdrv_reopen_prepare() has succeeded. > Otherwise, the block driver has no chance to undo what it has done in > its implementation of .bdrv_reopen_prepare(). > > To fix this, bdrv_reopen_prepare() has to call .bdrv_reopen_abort() if > it wants to return an error after .bdrv_reopen_prepare() has succeeded. > > Signed-off-by: Max Reitz <mre...@redhat.com> > --- > block.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/block.c b/block.c > index fd67e14dfa..7f5859aa74 100644 > --- a/block.c > +++ b/block.c > @@ -3332,7 +3332,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, > BlockReopenQueue *queue, > if (!qobject_is_equal(new, old)) { > error_setg(errp, "Cannot change the option '%s'", > entry->key); > ret = -EINVAL; > - goto error; > + goto late_error; > } > } while ((entry = qdict_next(reopen_state->options, entry))); > } > @@ -3340,7 +3340,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, > BlockReopenQueue *queue, > ret = bdrv_check_perm(reopen_state->bs, queue, reopen_state->perm, > reopen_state->shared_perm, NULL, errp); > if (ret < 0) { > - goto error; > + goto late_error; > } > > ret = 0; > @@ -3354,6 +3354,19 @@ error: > qobject_unref(orig_reopen_opts); > g_free(discard); > return ret; > + > +late_error: > + /* drv->bdrv_reopen_prepare() has succeeded, so we need to call > + * drv->bdrv_reopen_abort() before signaling an error > + * (bdrv_reopen_multiple() will not call bdrv_reopen_abort() when > + * the respective bdrv_reopen_prepare() failed) */ > + if (drv->bdrv_reopen_abort) { > + drv->bdrv_reopen_abort(reopen_state); > + } > + qemu_opts_del(opts); > + qobject_unref(orig_reopen_opts); > + g_free(discard); > + return ret; > }
Instead of having two exit points you could also have something like bool drv_prepared, set it to 'true' after drv->bdrv_reopen_prepare() has succeeded and then simply add this at the end: if (ret < 0 && drv_prepared) { drv->bdrv_reopen_abort(reopen_state); } Berto