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

[Kevin Wolf]

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 <address@hidden>

> +/*
> + * 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?

> +    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).

> +
> +    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.

> +    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.

> +}
> +
> +/*
> + * 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