Re: [Qemu-devel] [v2 Patch 1/9]block: Framework for reopening image files safely

[Kevin Wolf <kwolf@redhat.com>]

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