| Message ID | 20181116155302.22472-2-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix two minor reopen issues | expand |
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 <mreitz@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
On 16.11.18 17:02, Alberto Garcia wrote: > 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 <mreitz@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); > } Yup, sure. Max
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; } /*
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 <mreitz@redhat.com> --- block.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)