From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:48019) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RSo6i-0005Bd-1h for qemu-devel@nongnu.org; Tue, 22 Nov 2011 06:01:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RSo6d-0008T8-T1 for qemu-devel@nongnu.org; Tue, 22 Nov 2011 06:01:44 -0500 Received: from mx1.redhat.com ([209.132.183.28]:62357) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RSo6d-0008T1-Lw for qemu-devel@nongnu.org; Tue, 22 Nov 2011 06:01:39 -0500 Message-ID: <4ECB81A5.60600@redhat.com> Date: Tue, 22 Nov 2011 12:04:05 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <20111111064707.15024.69847.sendpatchset@skannery.in.ibm.com> <20111111064818.15024.2323.sendpatchset@skannery.in.ibm.com> <4ECA407A.6030507@in.ibm.com> <4ECB7866.50405@in.ibm.com> In-Reply-To: <4ECB7866.50405@in.ibm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: supriya kannery Cc: qemu-devel@nongnu.org, Stefan Hajnoczi , Luiz Capitulino , Christoph Hellwig , Supriya Kannery Am 22.11.2011 11:24, schrieb supriya kannery: > Stefan Hajnoczi wrote: >> On Mon, Nov 21, 2011 at 12:13 PM, supriya kannery wrote: >> >>> Stefan Hajnoczi wrote: >>> >>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery >>>> wrote: >>>> >>>> >>>>> @@ -708,17 +731,31 @@ 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_REOPEN_FILE_FAILED, bs->filename); >>>>> - ret = bdrv_open(bs, bs->filename, open_flags, drv); >>>>> + /* Use driver specific reopen() if available */ >>>>> + if (drv->bdrv_reopen_prepare) { >>>>> >>>>> >>>> This seems weird to me because we're saying a driver may have >>>> drv->bdrv_reopen_prepare == NULL but the public bdrv_reopen_prepare() >>>> function doesn't check and return -ENOTSUP. >>>> >>>> >>> If drv->bdrv_reopen_prepare == NULL , then we are not calling the >>> publick bdrv_reopen_prepare at all. Unless we later call public >>> bdrv_reopen_prepare >>> from elsewhere without checking drv->bdrv_reopen_prepare, a check for >>> -ENOTSUP inside the public one is not needed right? >>> >>> Also, we are handling reopening for even those drivers which don't >>> have its own bdrv_reopen_prepare defined, by taking the "else" >>> control path. So condition for reporting "ENOTSUP" shouldn't come >>> up as of now. Please let me know your thoughts. >>> >> >> How does VMDK implement its prepare/commit/abort? It needs to use the >> "public" bdrv_reopen_prepare() function on its image files. >> > > bdrv_reopen() is the public interface which gets called by any of the > image formats. > So VMDK or any image format has to call bdrv_reopen which decides to call > driver specific prepare/commit/abort or simply close and reopen the file. No, that doesn't work. In order to get all-or-nothing semantics, you need to explicitly prepare all child images and only when you know the results of all preparations, you can decide whether to commit or abort all. Kevin