[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/7] block: Framework for reopening files safely
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 2/7] block: Framework for reopening files safely |
Date: |
Wed, 05 Sep 2012 17:09:17 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120605 Thunderbird/13.0 |
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
- Re: [Qemu-devel] [PATCH 2/7] block: Framework for reopening files safely,
Kevin Wolf <=