From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:57657)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1SzOv9-0001nI-7y
	for qemu-devel@nongnu.org; Thu, 09 Aug 2012 05:20:51 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1SzOv4-0000Ei-5B
	for qemu-devel@nongnu.org; Thu, 09 Aug 2012 05:20:47 -0400
Received: from mx1.redhat.com ([209.132.183.28]:53906)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1SzOv3-0000EF-Pm
	for qemu-devel@nongnu.org; Thu, 09 Aug 2012 05:20:42 -0400
Message-ID: <502380DD.4060908@redhat.com>
Date: Thu, 09 Aug 2012 11:20:29 +0200
From: Kevin Wolf <kwolf@redhat.com>
MIME-Version: 1.0
References: <20120730213409.21536.7589.sendpatchset@skannery.in.ibm.com>
	<20120730213422.21536.81486.sendpatchset@skannery.in.ibm.com>
	<50233BE5.8070101@redhat.com>
In-Reply-To: <50233BE5.8070101@redhat.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [v2 Patch 1/9]block: Framework for reopening image
 files safely
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: jcody@redhat.com
Cc: Supriya Kannery <supriyak@linux.vnet.ibm.com>, Shrinidhi Joshi <spjoshi31@gmail.com>, Stefan Hajnoczi <stefanha@gmail.com>, qemu-devel@nongnu.org, Luiz Capitulino <lcapitulino@redhat.com>, Christoph Hellwig <hch@lst.de>

Am 09.08.2012 06:26, schrieb Jeff Cody:
> On 07/30/2012 05:34 PM, 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.
>>
>> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
>>
>> ---
>> Index: qemu/block.c
>> ===================================================================
>> --- qemu.orig/block.c
>> +++ qemu/block.c
>> @@ -859,6 +859,60 @@ 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);
>> +}
>> +
>> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    int ret = 0;
>> +    BDRVReopenState *reopen_state = NULL;
>> +
> 
> We should assert if drv is NULL:
> 
>     assert(drv != NULL);
> 
> 
>> +    /* Quiesce IO for the given block device */
>> +    bdrv_drain_all();
>> +    ret = bdrv_flush(bs);
>> +    if (ret != 0) {
>> +        error_set(errp, QERR_IO_ERROR);
>> +        return;
>> +    }
>> +
> 
> We also need to reopen bs->file, so maybe something like this:
> 
>     /* open any file images */
>     if (bs->file) {
>         bdrv_reopen(bs->file, bdrv_flags, errp);
>         if (errp && *errp) {
>             goto exit;
>         }
>     }
> 
> This will necessitate making the handlers in the raw.c file just stubs
> (I'll respond to that patch as well).

Doesn't this break the transactional semantics? I think you should only
prepare the bs->file reopen here and commit it when committing this one.

Kevin