[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/5] block: Simplify bdrv_reopen_abort()
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 3/5] block: Simplify bdrv_reopen_abort() |
Date: |
Tue, 14 Aug 2018 12:59:29 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
Am 14.08.2018 um 12:15 hat Alberto Garcia geschrieben:
> On Tue 14 Aug 2018 11:07:42 AM CEST, Kevin Wolf wrote:
> > Am 29.06.2018 um 13:37 hat Alberto Garcia geschrieben:
> >> If a bdrv_reopen_multiple() call fails, then the explicit_options
> >> QDict has to be deleted for every entry in the reopen queue. This must
> >> happen regardless of whether that entry's bdrv_reopen_prepare() call
> >> succeeded or not.
> >>
> >> This patch simplifies the cleanup code a bit.
> >>
> >> Signed-off-by: Alberto Garcia <address@hidden>
> >> ---
> >> block.c | 9 ++++-----
> >> 1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index 5aaed424b9..48e8f4814c 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -3060,9 +3060,10 @@ int bdrv_reopen_multiple(AioContext *ctx,
> >> BlockReopenQueue *bs_queue, Error **er
> >>
> >> cleanup:
> >> QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
> >> - if (ret && bs_entry->prepared) {
> >> - bdrv_reopen_abort(&bs_entry->state);
> >> - } else if (ret) {
> >> + if (ret) {
> >> + if (bs_entry->prepared) {
> >> + bdrv_reopen_abort(&bs_entry->state);
> >> + }
> >> qobject_unref(bs_entry->state.explicit_options);
> >> }
> >> qobject_unref(bs_entry->state.options);
> >> @@ -3351,8 +3352,6 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
> >> drv->bdrv_reopen_abort(reopen_state);
> >> }
> >>
> >> - qobject_unref(reopen_state->explicit_options);
> >> -
> >> bdrv_abort_perm_update(reopen_state->bs);
> >> }
> >
> > I think this is only correct if we make bdrv_reopen_prepare/commit/abort
> > private. At the moment they are public interfaces, so we must have
> > thought that some callers might want to integrate them into other
> > transactions.
>
> But that's not a problem as long as the new callers unref
> state.explicit_options when bdrv_reopen_prepare() fails. They need to do
> it with state.options anyway.
Actually that makes sense because the code creating the object isn't
even in bdrv_reopen_prepare(), but in bdrv_reopen_queue_child(). So it's
the caller that owns the object and should free them too. I just
confused the two functions.
Kevin