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

[supriya kannery <supriyak@in.ibm.com>]

Stefan Hajnoczi wrote:
> On Mon, Nov 21, 2011 at 12:13 PM, supriya kannery <supriyak@in.ibm.com> wrote:
>   
>> Stefan Hajnoczi wrote:
>>     
>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>> <supriyak@linux.vnet.ibm.com> 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
>
>