From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:34147) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RxIWu-0008Ad-0k for qemu-devel@nongnu.org; Tue, 14 Feb 2012 08:34:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RxIWo-0001AA-77 for qemu-devel@nongnu.org; Tue, 14 Feb 2012 08:34:47 -0500 Received: from e28smtp07.in.ibm.com ([122.248.162.7]:39483) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RxIWn-000183-KY for qemu-devel@nongnu.org; Tue, 14 Feb 2012 08:34:42 -0500 Received: from /spool/local by e28smtp07.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 14 Feb 2012 19:04:35 +0530 Received: from d28av02.in.ibm.com (d28av02.in.ibm.com [9.184.220.64]) by d28relay01.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q1EDYTtl4501600 for ; Tue, 14 Feb 2012 19:04:29 +0530 Received: from d28av02.in.ibm.com (loopback [127.0.0.1]) by d28av02.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q1EDYS0f030281 for ; Wed, 15 Feb 2012 00:34:28 +1100 Message-ID: <4F3A62E3.7030902@linux.vnet.ibm.com> Date: Tue, 14 Feb 2012 19:04:27 +0530 From: Supriya Kannery MIME-Version: 1.0 References: <20120201030557.2990.74150.sendpatchset@skannery.in.ibm.com> <20120201030658.2990.15176.sendpatchset@skannery.in.ibm.com> <20120207100836.GB29427@stefanha-thinkpad.localdomain> In-Reply-To: <20120207100836.GB29427@stefanha-thinkpad.localdomain> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely Reply-To: supriyak@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Christoph Hellwig , qemu-devel@nongnu.org, Luiz Capitulino On 02/07/2012 03:38 PM, Stefan Hajnoczi wrote: > On Wed, Feb 01, 2012 at 08:36:58AM +0530, Supriya Kannery wrote: >> 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. >> >> +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags) >> +{ >> + BlockDriver *drv = bs->drv; >> + >> + return drv->bdrv_reopen_prepare(bs, prs, flags); > > Indentation should be 4 spaces. I suggest configuring your editor so > this is always correct and done automatically. > ok >> - 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) { >> + ret = bdrv_reopen_prepare(bs,&reopen_state, bdrv_flags); >> + if (ret< 0) { > > Indentation > >> + bdrv_reopen_abort(bs, reopen_state); >> + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); >> + return ret; >> + } >> + >> + bdrv_reopen_commit(bs, reopen_state); >> + bs->open_flags = bdrv_flags; > > Is bs->open_flags not assigned inside bdrv_reopen_*()? > No, Since it is generic for all the drivers, placed it here. >> + >> + } else { >> + open_flags = bs->open_flags; >> + bdrv_close(bs); >> + >> + ret = bdrv_open(bs, bs->filename, bdrv_flags, drv); >> if (ret< 0) { >> - /* Reopen failed with orig and modified flags */ >> - abort(); >> + /* 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); >> + if (ret< 0) { >> + /* Reopen failed with orig and modified flags */ >> + bs->drv = NULL; > > This should be a post-condition of bdrv_open(). If you have found a > case where bs->drv != NULL after bdrv_open() fails, then please fix that > code path instead of assigning NULL here. > ok, will check on this -thanks for reviewing, Supriya