[v1,4/10] Qemu: Framework for reopening image files safely

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.

Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>

On 06/15/2012 02:47 PM, 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.
> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
> 

> +    /* Use driver specific reopen() if available */
> +    if (drv->bdrv_reopen_prepare) {
> +        ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
> +         if (ret < 0) {
> +            bdrv_reopen_abort(bs, reopen_state);
> +            qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
> +            return ret;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state);
> +        bs->open_flags = bdrv_flags;

Good.

> +
> +    } else {
> +
> +        /* Try reopening the image using protocol or directly */
> +        if (bs->file) {
> +            open_flags = bs->open_flags;
> +            drv->bdrv_close(bs);
> +            if (drv->bdrv_file_open) {
> +                ret = drv->bdrv_file_open(bs, bs->filename, bdrv_flags);

Not so good.  Are we sure we can't flush all data, so that we can then
attempt the open before the close, and avoid losing the original file on
error?

> +            } else {
> +                ret = bdrv_file_open(&bs->file, bs->filename, bdrv_flags);
> +            }
> +            if (ret < 0) {
> +                /* Reopen failed. Try to open with original flags */
> +                qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
> +                if (drv->bdrv_file_open) {
> +                    ret = drv->bdrv_file_open(bs, bs->filename, open_flags);
> +                } else {
> +                    ret = bdrv_file_open(&bs->file, bs->filename, open_flags);
> +                }
> +                if (ret < 0) {
> +                    /* Reopen failed with orig and modified flags */
> +                    bdrv_delete(bs->file);
> +                    bs->drv = NULL;

You dropped the abort() here, but now the device is silently gone.  But
since the error of QERR_OPEN_FILE_FAILED is the same as in the safe
case, how does the management app know that things were corrupted?

Am 15.06.2012 22:47, schrieb Supriya Kannery:
> 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.
> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
> 
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -858,10 +858,32 @@ unlink_and_fail:
>      return ret;
>  }
>  
> +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
> +{
> +     BlockDriver *drv = bs->drv;
> +
> +     return drv->bdrv_reopen_prepare(bs, prs, flags);
> +}
> +
> +void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    drv->bdrv_reopen_commit(bs, rs);
> +}
> +
> +void bdrv_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    drv->bdrv_reopen_abort(bs, rs);
> +}
> +
>  int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
>  {
>      BlockDriver *drv = bs->drv;
>      int ret = 0, open_flags;
> +    BDRVReopenState *reopen_state = NULL;
>  
>      /* Quiesce IO for the given block device */
>      qemu_aio_flush();
> @@ -870,17 +892,44 @@ 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_OPEN_FILE_FAILED, bs->filename);
> -        ret = bdrv_open(bs, bs->filename, open_flags, drv);
> -        if (ret < 0) {
> -            /* Reopen failed with orig and modified flags */
> -            abort();
> +    /* Use driver specific reopen() if available */
> +    if (drv->bdrv_reopen_prepare) {
> +        ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
> +         if (ret < 0) {
> +            bdrv_reopen_abort(bs, reopen_state);
> +            qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
> +            return ret;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state);
> +        bs->open_flags = bdrv_flags;
> +
> +    } else {
> +
> +        /* Try reopening the image using protocol or directly */
> +        if (bs->file) {
> +            open_flags = bs->open_flags;
> +            drv->bdrv_close(bs);
> +            if (drv->bdrv_file_open) {
> +                ret = drv->bdrv_file_open(bs, bs->filename, bdrv_flags);
> +            } else {
> +                ret = bdrv_file_open(&bs->file, bs->filename, bdrv_flags);

Doesn't this forget to reopen bs itself? And bs->file wasn't even
closed. If think we need something more along the lines of:

if (bs->file) {
    bdrv_reopen(bs->file)
}

if (drv->bdrv_file_open) {
    drv->bdrv_file_open(bs)
} else {
   drv->bdrv_open(bs)
}

In fact we would really want to be able to commit/abort the bdrv_reopen
of bs->file only after we know if bdrv_open(bs) has succeeded, but with
the current design we can't because bdrv_open wants to read from the new fd.

Maybe it would make sense to require bdrv_reopen_prepare() to do the
switch without throwing the old state away yet, but it sounds as if it
would make implementations for image formats quite a bit harder.

Maybe we should completely avoid this default implementation and just
fail bdrv_reopen if a driver doesn't support the explicit,
transactionable reopen functions.

Kevin