From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56014) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SzPBw-0006Zf-2c for qemu-devel@nongnu.org; Thu, 09 Aug 2012 05:38:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SzPBu-0005kn-UX for qemu-devel@nongnu.org; Thu, 09 Aug 2012 05:38:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24059) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SzPBu-0005ki-MQ for qemu-devel@nongnu.org; Thu, 09 Aug 2012 05:38:06 -0400 Message-ID: <502384F6.4040703@redhat.com> Date: Thu, 09 Aug 2012 11:37:58 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <20120730213409.21536.7589.sendpatchset@skannery.in.ibm.com> <20120730213525.21536.9516.sendpatchset@skannery.in.ibm.com> <50233BF4.90505@redhat.com> In-Reply-To: <50233BF4.90505@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v2 Patch 5/9]block: qcow2 image file reopen List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: jcody@redhat.com Cc: Supriya Kannery , Shrinidhi Joshi , Stefan Hajnoczi , qemu-devel@nongnu.org, Luiz Capitulino , Christoph Hellwig Am 09.08.2012 06:26, schrieb Jeff Cody: > On 07/30/2012 05:35 PM, Supriya Kannery wrote: >> qcow2 driver changes for bdrv_reopen_xx functions to >> safely reopen image files. Reopening of image files while >> changing hostcache dynamically is handled here. >> >> Signed-off-by: Supriya Kannery >> >> --- >> Index: qemu/block/qcow2.c >> =================================================================== >> --- qemu.orig/block/qcow2.c >> +++ qemu/block/qcow2.c >> @@ -52,10 +52,19 @@ typedef struct { >> uint32_t magic; >> uint32_t len; >> } QCowExtension; >> + >> +typedef struct BDRVQcowReopenState { >> + BDRVReopenState reopen_state; >> + BDRVQcowState *stash_s; >> +} BDRVQcowReopenState; >> + >> #define QCOW2_EXT_MAGIC_END 0 >> #define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA >> #define QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857 >> >> +static void qcow2_stash_state(BDRVQcowState *stashed_state, BDRVQcowState *s); >> +static void qcow2_revert_state(BDRVQcowState *s, BDRVQcowState *stashed_state); >> + >> static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename) >> { >> const QCowHeader *cow_header = (const void *)buf; >> @@ -434,6 +443,169 @@ static int qcow2_open(BlockDriverState * >> return ret; >> } >> >> +static int qcow2_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, >> + int flags) >> +{ >> + BDRVQcowReopenState *qcow2_rs = g_malloc0(sizeof(BDRVQcowReopenState)); >> + int ret = 0; >> + BDRVQcowState *s = bs->opaque; >> + >> + qcow2_rs->reopen_state.bs = bs; >> + >> + /* save state before reopen */ >> + qcow2_rs->stash_s = g_malloc0(sizeof(BDRVQcowState)); >> + qcow2_stash_state(qcow2_rs->stash_s, s); >> + *prs = &(qcow2_rs->reopen_state); >> + >> + /* Reopen file with new flags */ >> + ret = qcow2_open(bs, flags); >> + return ret; >> +} >> + >> +static void qcow2_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs) >> +{ >> + BDRVQcowReopenState *qcow2_rs; >> + BDRVQcowState *stashed_s; >> + >> + qcow2_rs = container_of(rs, BDRVQcowReopenState, reopen_state); >> + stashed_s = qcow2_rs->stash_s; >> + >> + qcow2_cache_flush(bs, stashed_s->l2_table_cache); >> + qcow2_cache_flush(bs, stashed_s->refcount_block_cache); >> + >> + qcow2_cache_destroy(bs, stashed_s->l2_table_cache); >> + qcow2_cache_destroy(bs, stashed_s->refcount_block_cache); >> + >> + g_free(stashed_s->unknown_header_fields); >> + cleanup_unknown_header_ext(bs); >> + >> + g_free(stashed_s->cluster_cache); >> + qemu_vfree(stashed_s->cluster_data); > > > >> + qcow2_refcount_close(bs); >> + qcow2_free_snapshots(bs); > > I assume these are unintentional? I believe this is what is causing your > double-free issue. This whole patch looks a bit complicated and brittle. What qcow2 state do you actually expect to change with a new qcow2_open()? Is it even required to do more than just modifying bs->file? qcow2 doesn't really do anything with the flags. Kevin