Re: [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely

[Supriya Kannery <supriyak@linux.vnet.ibm.com>]

On 02/07/2012 03:38 PM, Stefan Hajnoczi wrote:
> On Wed, Feb 01, 2012 at 08:36:58AM +0530, Supriya Kannery wrote:
>> Struct BDRVReopenState along with three reopen related functions
>> introduced for handling reopening of images safely. This can be
>> extended by each of the block drivers to reopen respective
>> image files.
>>

>> +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
>> +{
>> +     BlockDriver *drv = bs->drv;
>> +
>> +     return drv->bdrv_reopen_prepare(bs, prs, flags);
>
> Indentation should be 4 spaces.  I suggest configuring your editor so
> this is always correct and done automatically.
>

ok

>> -    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) {
>> +        ret = bdrv_reopen_prepare(bs,&reopen_state, bdrv_flags);
>> +         if (ret<  0) {
>
> Indentation
>
>> +            bdrv_reopen_abort(bs, reopen_state);
>> +            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
>> +            return ret;
>> +        }
>> +
>> +        bdrv_reopen_commit(bs, reopen_state);
>> +        bs->open_flags = bdrv_flags;
>
> Is bs->open_flags not assigned inside bdrv_reopen_*()?
>

No, Since it is generic for all the drivers, placed it here.

>> +
>> +    } else {
>> +       open_flags = bs->open_flags;
>> +       bdrv_close(bs);
>> +
>> +       ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
>>           if (ret<  0) {
>> -            /* Reopen failed with orig and modified flags */
>> -            abort();
>> +            /* 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);
>> +            if (ret<  0) {
>> +                /* Reopen failed with orig and modified flags */
>> +                bs->drv = NULL;
>
> This should be a post-condition of bdrv_open().  If you have found a
> case where bs->drv != NULL after bdrv_open() fails, then please fix that
> code path instead of assigning NULL here.
>

ok, will check on this

-thanks for reviewing, Supriya