From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48682) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SoFYR-0003U5-JE for qemu-devel@nongnu.org; Mon, 09 Jul 2012 11:07:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SoFYH-0000hQ-GF for qemu-devel@nongnu.org; Mon, 09 Jul 2012 11:07:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:17837) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SoFYH-0000h9-7z for qemu-devel@nongnu.org; Mon, 09 Jul 2012 11:07:05 -0400 Message-ID: <4FFAF390.8090107@redhat.com> Date: Mon, 09 Jul 2012 17:06:56 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <20120615204648.9853.1225.sendpatchset@skannery.in.ibm.com> <20120615204744.9853.62830.sendpatchset@skannery.in.ibm.com> In-Reply-To: <20120615204744.9853.62830.sendpatchset@skannery.in.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v1 Patch 4/10]Qemu: Framework for reopening image files safely List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Supriya Kannery Cc: Shrinidhi Joshi , Stefan Hajnoczi , Jeff Cody , qemu-devel@nongnu.org, Luiz Capitulino , Christoph Hellwig Am 15.06.2012 22:47, schrieb Supriya Kannery: > Struct BDRVReopenState along with three reopen related functions > introduced for handling reopening of images safely. This can be > extended by each of the block drivers to reopen respective > image files. > > Signed-off-by: Supriya Kannery > > Index: qemu/block.c > =================================================================== > --- qemu.orig/block.c > +++ qemu/block.c > @@ -858,10 +858,32 @@ unlink_and_fail: > return ret; > } > > +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags) > +{ > + BlockDriver *drv = bs->drv; > + > + return drv->bdrv_reopen_prepare(bs, prs, flags); > +} > + > +void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs) > +{ > + BlockDriver *drv = bs->drv; > + > + drv->bdrv_reopen_commit(bs, rs); > +} > + > +void bdrv_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs) > +{ > + BlockDriver *drv = bs->drv; > + > + drv->bdrv_reopen_abort(bs, rs); > +} > + > int bdrv_reopen(BlockDriverState *bs, int bdrv_flags) > { > BlockDriver *drv = bs->drv; > int ret = 0, open_flags; > + BDRVReopenState *reopen_state = NULL; > > /* Quiesce IO for the given block device */ > qemu_aio_flush(); > @@ -870,17 +892,44 @@ int bdrv_reopen(BlockDriverState *bs, in > qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name); > return ret; > } > - open_flags = bs->open_flags; > - bdrv_close(bs); > > - ret = bdrv_open(bs, bs->filename, bdrv_flags, drv); > - if (ret < 0) { > - /* Reopen failed. Try to open with original flags */ > - qerror_report(QERR_OPEN_FILE_FAILED, bs->filename); > - ret = bdrv_open(bs, bs->filename, open_flags, drv); > - if (ret < 0) { > - /* Reopen failed with orig and modified flags */ > - abort(); > + /* Use driver specific reopen() if available */ > + if (drv->bdrv_reopen_prepare) { > + ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags); > + if (ret < 0) { > + bdrv_reopen_abort(bs, reopen_state); > + qerror_report(QERR_OPEN_FILE_FAILED, bs->filename); > + return ret; > + } > + > + bdrv_reopen_commit(bs, reopen_state); > + bs->open_flags = bdrv_flags; > + > + } else { > + > + /* Try reopening the image using protocol or directly */ > + if (bs->file) { > + open_flags = bs->open_flags; > + drv->bdrv_close(bs); > + if (drv->bdrv_file_open) { > + ret = drv->bdrv_file_open(bs, bs->filename, bdrv_flags); > + } else { > + ret = bdrv_file_open(&bs->file, bs->filename, bdrv_flags); Doesn't this forget to reopen bs itself? And bs->file wasn't even closed. If think we need something more along the lines of: if (bs->file) { bdrv_reopen(bs->file) } if (drv->bdrv_file_open) { drv->bdrv_file_open(bs) } else { drv->bdrv_open(bs) } In fact we would really want to be able to commit/abort the bdrv_reopen of bs->file only after we know if bdrv_open(bs) has succeeded, but with the current design we can't because bdrv_open wants to read from the new fd. Maybe it would make sense to require bdrv_reopen_prepare() to do the switch without throwing the old state away yet, but it sounds as if it would make implementations for image formats quite a bit harder. Maybe we should completely avoid this default implementation and just fail bdrv_reopen if a driver doesn't support the explicit, transactionable reopen functions. Kevin