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