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

[Supriya Kannery]

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