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

[Kevin Wolf <kwolf@redhat.com>]

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