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

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

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.

> This check can be moved into bdrv_reopen_prepare().  We can test for
> the -ENOTSUP return value here instead.
>
>   
>> +        ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
>> +         if (ret < 0) {
>>     
>
> Indentation is off here.
>   

sure..will take care in next version.
> Stefan
>
>