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

[Supriya Kannery]

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<address@hidden>
>
> 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