From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46558) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RST5Y-0006Hv-B3 for qemu-devel@nongnu.org; Mon, 21 Nov 2011 07:35:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RST5W-0005kl-2e for qemu-devel@nongnu.org; Mon, 21 Nov 2011 07:35:08 -0500 Received: from e28smtp04.in.ibm.com ([122.248.162.4]:42582) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RST5U-0005h5-Hs for qemu-devel@nongnu.org; Mon, 21 Nov 2011 07:35:06 -0500 Received: from /spool/local by e28smtp04.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 21 Nov 2011 18:04:53 +0530 Received: from d28av03.in.ibm.com (d28av03.in.ibm.com [9.184.220.65]) by d28relay01.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pALCYlsE4141088 for ; Mon, 21 Nov 2011 18:04:48 +0530 Received: from d28av03.in.ibm.com (loopback [127.0.0.1]) by d28av03.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pALCYlQM008334 for ; Mon, 21 Nov 2011 23:34:47 +1100 Message-ID: <4ECA407A.6030507@in.ibm.com> Date: Mon, 21 Nov 2011 17:43:46 +0530 From: supriya kannery MIME-Version: 1.0 References: <20111111064707.15024.69847.sendpatchset@skannery.in.ibm.com> <20111111064818.15024.2323.sendpatchset@skannery.in.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: Stefan Hajnoczi Cc: Kevin Wolf , Supriya Kannery , Luiz Capitulino , Christoph Hellwig , qemu-devel@nongnu.org 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. > This check can be moved into bdrv_reopen_prepare(). We can test for > the -ENOTSUP return value here instead. > > >> + ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags); >> + if (ret < 0) { >> > > Indentation is off here. > sure..will take care in next version. > Stefan > >