From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:58437) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RSntN-0000F7-CE for qemu-devel@nongnu.org; Tue, 22 Nov 2011 05:47:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RSntL-0005Ut-KU for qemu-devel@nongnu.org; Tue, 22 Nov 2011 05:47:57 -0500 Received: from e28smtp06.in.ibm.com ([122.248.162.6]:45795) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RSntK-0005Uj-CA for qemu-devel@nongnu.org; Tue, 22 Nov 2011 05:47:55 -0500 Received: from /spool/local by e28smtp06.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 22 Nov 2011 16:16:57 +0530 Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay01.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pAMAkiOh4329690 for ; Tue, 22 Nov 2011 16:16:45 +0530 Received: from d28av01.in.ibm.com (loopback [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pAMAjekV009582 for ; Tue, 22 Nov 2011 16:15:40 +0530 Message-ID: <4ECB7866.50405@in.ibm.com> Date: Tue, 22 Nov 2011 15:54:38 +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> <4ECA407A.6030507@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 , qemu-devel@nongnu.org, Christoph Hellwig , Luiz Capitulino 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. > BTW I think the bdrv_reopen_*() functions should go in block_int.h and > not block.h. They are visible to the block layer but not public to > the rest of QEMU, which must use the bdrv_reopen() interface only. > > I think what's really missing is a way to tie this all together. You > have posted raw format and raw-posix protocol patches. But we need to > cover image formats, where VMDK is the multi-file special case and > qcow2/qed/etc are simpler but also need to be supported. > > Right now anything but raw-posix is still closing and reopening. By > adding support for image formats I think you'll find the right way to > structure this code. > > Since only bdrv_reopen() is public, it is declared in block.h and structure of code done in similar way how bdrv_open() is done. The else part in bdrv_reopen() will handle reopen requests for images other than raw for now (simply close and reopen). -thanks, Supriya > Stefan > >