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

[Stefan Hajnoczi]

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.

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.

Stefan