Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen

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

On 10/12/2011 08:25 PM, Kevin Wolf wrote:
> Am 11.10.2011 05:11, schrieb Supriya Kannery:
>> Struct BDRVReopenState introduced for handling reopen state of images.
>> This can be  extended by each of the block drivers to reopen respective
>> image files. Implementation for raw-posix is done here.
>>
>> Signed-off-by: Supriya Kannery<supriyak@linux.vnet.ibm.com>
>
> Maybe it would make sense to split this into two patches, one for the
> block.c infrastructure and another one for adding the callbacks in drivers.
>

ok..will split in v8.

>> +
>> +    /* If only O_DIRECT to be toggled, use fcntl */
>> +    if (!((bs->open_flags&  ~BDRV_O_NOCACHE) ^
>> +            (flags&  ~BDRV_O_NOCACHE))) {
>> +        raw_rs->reopen_fd = dup(s->fd);
>> +        if (raw_rs->reopen_fd<= 0) {
>> +            return -1;
>
> This leaks raw_rs.
>

will handle

>> +        }
>> +    }
>> +
>> +    /* TBD: Handle O_DSYNC and other flags */
>> +    *rs = raw_rs;
>> +    return 0;
>> +}
>> +
>> +static int raw_reopen_commit(BDRVReopenState *rs)
>
> bdrv_reopen_commit must never fail. Any error that can happen must
> already be handled in bdrv_reopen_prepare.
>
> The commit function should really only do s->fd = rs->reopen_fd (besides
> cleanup), everything else should already be prepared.
>

will move call to fcntl to bdrv_reopen_prepare.

>> +{
>> +    BlockDriverState *bs = rs->bs;
>> +    BDRVRawState *s = bs->opaque;
>> +
>> +    if (!rs->reopen_fd) {
>> +        return -1;
>> +    }
>> +
>> +    int ret = fcntl_setfl(rs->reopen_fd, rs->reopen_flags);
>
> reopen_flags is BDRV_O_*, not O_*, so it needs to be translated.
>

ok

>> +    /* Use driver specific reopen() if available */
>> +    if (drv->bdrv_reopen_prepare) {
>> +        ret = drv->bdrv_reopen_prepare(bs,&rs, bdrv_flags);
>>           if (ret<  0) {
>> -            /* Reopen failed with orig and modified flags */
>> -            abort();
>> +            goto fail;
>>           }
>> -    }
>> +        if (drv->bdrv_reopen_commit) {
>> +            ret = drv->bdrv_reopen_commit(rs);
>> +            if (ret<  0) {
>> +                goto fail;
>> +            }
>> +            return 0;
>> +        }
>
> Pull the return 0; out one level. It would be really strange if we
> turned a successful prepare into reopen_abort just because the driver
> doesn't need a commit function.
>
> (The other consistent way would be to require that if a driver
> implements one reopen function, it has to implement all three of them.
> I'm fine either way.)
>

Will give flexibility to drivers, not mandating all the three functions.

>> +            ret = bdrv_open(bs, bs->filename, open_flags, drv);
>> +            if (ret<  0) {
>> +                /*
>> +                 * Reopen with orig and modified flags failed
>> +                 */
>> +                abort();
>
> Make this bs->drv = NULL, so that trying to access to image will fail,
> but at least not the whole VM crashes.
>

ok

>>   static int raw_read(BlockDriverState *bs, int64_t sector_num,
>>                       uint8_t *buf, int nb_sectors)
>>   {
>> @@ -128,6 +137,8 @@ static BlockDriver bdrv_raw = {
>>       .instance_size      = 1,
>>
>>       .bdrv_open          = raw_open,
>> +    .bdrv_reopen_prepare
>> +                        = raw_reopen,
>>       .bdrv_close         = raw_close,
>>       .bdrv_read          = raw_read,
>>       .bdrv_write         = raw_write,
>
> I think raw must pass through all three functions. Otherwise it could
> happen that we need to abort, but the image has already been reopened.
>

ok..got it..will have three separate functions to avoid unnecessary
dependencies.

Got a question..
    In raw-posix, the three functions are implemented for file_reopen 
for now. Should this be extended to hdev, cdrom and floppy?

> Kevin
>