From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33041) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9HEq-0007IA-Pk for qemu-devel@nongnu.org; Wed, 05 Sep 2012 11:09:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T9HEj-0003j2-MB for qemu-devel@nongnu.org; Wed, 05 Sep 2012 11:09:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43270) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9HEj-0003iu-DV for qemu-devel@nongnu.org; Wed, 05 Sep 2012 11:09:49 -0400 Message-ID: <50476B1D.7070406@redhat.com> Date: Wed, 05 Sep 2012 17:09:17 +0200 From: Kevin Wolf MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/7] block: Framework for reopening files safely List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody Cc: supriyak@linux.vnet.ibm.com, pbonzini@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, stefanha@gmail.com 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 > +/* > + * 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