Re: [Qemu-devel] [PATCH 2/7] block: Framework for reopening files safely

[Jeff Cody <jcody@redhat.com>]

On 09/05/2012 11:09 AM, Kevin Wolf wrote:
> Am 30.08.2012 20:47, schrieb Jeff Cody:
>> This is based heavily on Supriya Kannery's bdrv_reopen()
>> patch series.
>>
>> This provides a transactional method to reopen multiple
>> images files safely.
>>
>> Image files are queue for reopen via bdrv_reopen_queue(), and the
>> reopen occurs when bdrv_reopen_multiple() is called.  Changes are
>> staged in bdrv_reopen_prepare() and in the equivalent driver level
>> functions.  If any of the staged images fails a prepare, then all
>> of the images left untouched, and the staged changes for each image
>> abandoned.
>>
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
> 
>> +/*
>> + * Reopen multiple BlockDriverStates atomically & transactionally.
>> + *
>> + * The queue passed in (bs_queue) must have been built up previous
>> + * via bdrv_reopen_queue().
>> + *
>> + * Reopens all BDS specified in the queue, with the appropriate
>> + * flags.  All devices are prepared for reopen, and failure of any
>> + * device will cause all device changes to be abandonded, and intermediate
>> + * data cleaned up.
>> + *
>> + * If all devices prepare successfully, then the changes are committed
>> + * to all devices.
>> + *
>> + */
>> +int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
>> +{
>> +    int ret = -1;
>> +    BlockReopenQueueEntry *bs_entry;
>> +    Error *local_err = NULL;
>> +
>> +    assert(bs_queue != NULL);
>> +
>> +    bdrv_drain_all();
>> +
>> +    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
>> +        if (bdrv_reopen_prepare(bs_entry->state, &local_err)) {
>> +            error_propagate(errp, local_err);
>> +            goto cleanup;
>> +        }
>> +        bs_entry->prepared = true;
>> +    }
>> +
>> +    /* If we reach this point, we have success and just need to apply the
>> +     * changes
>> +     */
>> +    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
>> +        bdrv_reopen_commit(bs_entry->state);
>> +    }
>> +
>> +    ret = 0;
>> +
>> +cleanup:
>> +    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
>> +        if (ret && bs_entry->prepared) {
>> +            bdrv_reopen_abort(bs_entry->state);
>> +        }
>> +        g_free(bs_entry->state);
>> +        g_free(bs_entry);
>> +    }
> 
> Without QSIMPLEQ_FOREACH_SAFE, isn't this a use after free?
> 

Yes - that needs to be a QSIMPLEQ_FOREACH_SAFE.


>> +    g_free(bs_queue);
>> +    return ret;
>> +}
>> +
>> +
>> +/* Reopen a single BlockDriverState with the specified flags. */
>> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
>> +{
>> +    int ret = -1;
>> +    Error *local_err = NULL;
>> +    BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, bdrv_flags);
>> +
>> +    ret = bdrv_reopen_multiple(queue, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(errp, local_err);
>> +    }
>> +    return ret;
>> +}
>> +
>> +
>> +/*
>> + * Prepares a BlockDriverState for reopen. All changes are staged in the
>> + * 'reopen_state' field of the BlockDriverState, which must be NULL when
>> + * entering (all previous reopens must have completed for the BDS).
>> + *
>> + * bs is the BlockDriverState to reopen
>> + * flags are the new open flags
>> + *
>> + * Returns 0 on success, non-zero on error.  On error errp will be set
>> + * as well.
>> + *
>> + * On failure, bdrv_reopen_abort() will be called to clean up any data.
>> + * It is the responsibility of the caller to then call the abort() or
>> + * commit() for any other BDS that have been left in a prepare() state
>> + *
>> + */
>> +int bdrv_reopen_prepare(BDRVReopenState *reopen_state, Error **errp)
>> +{
>> +    int ret = -1;
>> +    Error *local_err = NULL;
>> +    BlockDriver *drv;
>> +
>> +    assert(reopen_state != NULL);
>> +    assert(reopen_state->bs->drv != NULL);
>> +    drv = reopen_state->bs->drv;
>> +
>> +    /* if we are to stay read-only, do not allow permission change
>> +     * to r/w */
>> +    if (reopen_state->bs->keep_read_only &&
> 
> Just for completeness, we decided to use the flag here instead of
> keep_read_only.
> 
>> +        reopen_state->flags & BDRV_O_RDWR) {
>> +        error_set(errp, QERR_DEVICE_IS_READ_ONLY,
>> +                  reopen_state->bs->device_name);
>> +        goto error;
>> +    }
>> +
>> +
>> +    ret = bdrv_flush(reopen_state->bs);
>> +    if (ret) {
>> +        error_set(errp, QERR_IO_ERROR);
>> +        goto error;
>> +    }
> 
> This throws the error code away. Bad. We should probably change
> QERR_IO_ERROR so that you can include strerror(-ret).
> 

Or, I could use the new error_setg() here, and pass along all relevant
information.

>> +
>> +    if (drv->bdrv_reopen_prepare) {
>> +        ret = drv->bdrv_reopen_prepare(reopen_state, &local_err);
>> +        if (ret) {
>> +            if (local_err != NULL) {
>> +                error_propagate(errp, local_err);
>> +            } else {
>> +                error_set(errp, QERR_OPEN_FILE_FAILED,
>> +                          reopen_state->bs->filename);
>> +            }
>> +            goto error;
>> +        }
>> +    } else {
>> +        /* It is currently mandatory to have a bdrv_reopen_prepare()
>> +         * handler for each supported drv. */
>> +        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
>> +                  drv->format_name, reopen_state->bs->device_name,
>> +                 "reopening of file");
>> +        ret = -1;
>> +        goto error;
>> +    }
>> +
>> +    return 0;
>> +
>> +error:
>> +    bdrv_reopen_abort(reopen_state);
> 
> This is unexpected for me. Shouldn't .bdrv_reopen_prepare() clean up
> before returning an error, like any other function does? (Which could
> actually be a call to bdrv_reopen_abort() where it makes sense.)
> 
> If you use .bdrv_reopen_abort() for it, block drivers must take care to
> write this function in way that doesn't assume that
> .bdrv_reopen_prepare() has completed. Sounds rather nasty to me.
> 

Hmm.  Yes, I can see that - and there is no cleanup this function needs
to do itself, so it can just assume that .bdrv_reopen_prepare() will
clean up after itself (which, as you mentioned, will likely be the
driver calling its own .bdrv_reopen_abort()).

Although, I think it should always be good form for the block drivers to
not make such assumptions, if possible.


>> +    return ret;
>> +}
>> +
>> +/*
>> + * Takes the staged changes for the reopen from bdrv_reopen_prepare(), and
>> + * makes them final by swapping the staging BlockDriverState contents into
>> + * the active BlockDriverState contents.
>> + */
>> +void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>> +{
>> +    BlockDriver *drv;
>> +
>> +    assert(reopen_state != NULL);
>> +    drv = reopen_state->bs->drv;
>> +    assert(drv != NULL);
>> +
>> +    /* If there are any driver level actions to take */
>> +    if (drv->bdrv_reopen_commit) {
>> +        drv->bdrv_reopen_commit(reopen_state);
>> +    }
>> +
>> +    /* set BDS specific flags now */
>> +    reopen_state->bs->open_flags         = reopen_state->flags;
>> +    reopen_state->bs->enable_write_cache = !!(reopen_state->flags &
>> +                                              BDRV_O_CACHE_WB);
>> +    reopen_state->bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
> 
> Hm, I wonder if these three lines can somehow be shared with the normal
> bdrv_open so that they stay in sync.

Sure, especially setting enable_write_cache.

> 
>> +}
>> +
>> +/*
>> + * Abort the reopen, and delete and free the staged changes in
>> + * reopen_state
>> + */
>> +void bdrv_reopen_abort(BDRVReopenState *reopen_state)
>> +{
>> +    BlockDriver *drv;
>> +
>> +    assert(reopen_state != NULL);
>> +    drv = reopen_state->bs->drv;
>> +    assert(drv != NULL);
>> +
>> +    if (drv->bdrv_reopen_abort) {
>> +        drv->bdrv_reopen_abort(reopen_state);
>> +    }
>> +}
>> +
>> +
>>  void bdrv_close(BlockDriverState *bs)
>>  {
>>      bdrv_flush(bs);
>> diff --git a/block.h b/block.h
>> index 4d919c2..db812b1 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -97,6 +97,14 @@ typedef enum {
>>      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>>  } BlockQMPEventAction;
>>  
>> +typedef struct BlockReopenQueueEntry {
>> +     bool prepared;
>> +     BDRVReopenState *state;
> 
> As discussed on IRC, this can be directly embedded instead of using a
> pointer.
> 
>> +     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
>> +} BlockReopenQueueEntry;
> 
> 
> Kevin
>