From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38619) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9Id0-0002ny-5I for qemu-devel@nongnu.org; Wed, 05 Sep 2012 12:38:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T9Icy-0007IP-5j for qemu-devel@nongnu.org; Wed, 05 Sep 2012 12:38:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49926) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9Icx-0007IK-TW for qemu-devel@nongnu.org; Wed, 05 Sep 2012 12:38:56 -0400 Message-ID: <50478019.60405@redhat.com> Date: Wed, 05 Sep 2012 12:38:49 -0400 From: Jeff Cody MIME-Version: 1.0 References: <50476B1D.7070406@redhat.com> In-Reply-To: <50476B1D.7070406@redhat.com> 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 Reply-To: jcody@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: supriyak@linux.vnet.ibm.com, pbonzini@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, stefanha@gmail.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 > >> +/* >> + * 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 >