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 <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);
> } Yup, sure. Max
signature.asc
Description: OpenPGP digital signature
