Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely

[Kevin Wolf]

Am 22.11.2011 11:24, schrieb supriya kannery:
> Stefan Hajnoczi wrote:
>> On Mon, Nov 21, 2011 at 12:13 PM, supriya kannery <address@hidden> wrote:
>>   
>>> Stefan Hajnoczi wrote:
>>>     
>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>>> <address@hidden> 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.

No, that doesn't work. In order to get all-or-nothing semantics, you
need to explicitly prepare all child images and only when you know the
results of all preparations, you can decide whether to commit or abort all.

Kevin