Re: [Qemu-devel] [PATCH for-3.1? 1/3] block: Always abort reopen after prepare succeeded

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3085 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]