Re: [Qemu-devel] [v2 Patch 1/9]block: Framework for reopening image files safely

[Jeff Cody]

On 07/30/2012 05:34 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 <address@hidden>
> 
> ---
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -859,6 +859,60 @@ 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);
> +}
> +
> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret = 0;
> +    BDRVReopenState *reopen_state = NULL;
> +

We should assert if drv is NULL:

    assert(drv != NULL);


> +    /* Quiesce IO for the given block device */
> +    bdrv_drain_all();
> +    ret = bdrv_flush(bs);
> +    if (ret != 0) {
> +        error_set(errp, QERR_IO_ERROR);
> +        return;
> +    }
> +

We also need to reopen bs->file, so maybe something like this:

    /* open any file images */
    if (bs->file) {
        bdrv_reopen(bs->file, bdrv_flags, errp);
        if (errp && *errp) {
            goto exit;
        }
    }

This will necessitate making the handlers in the raw.c file just stubs
(I'll respond to that patch as well).


> +    /* 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);
> +            error_set(errp, QERR_OPEN_FILE_FAILED, bs->filename);
> +            return;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state);
> +        bs->open_flags = bdrv_flags;
> +    } else {
> +        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
> +                  drv->format_name, bs->device_name,
> +                  "reopening of file");
> +        return;
> +    }
> +}
> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      bdrv_flush(bs);
> Index: qemu/block_int.h
> ===================================================================
> --- qemu.orig/block_int.h
> +++ qemu/block_int.h
> @@ -136,6 +136,14 @@ struct BlockDriver {
>      int instance_size;
>      int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char 
> *filename);
>      int (*bdrv_probe_device)(const char *filename);
> +
> +    /* For handling image reopen for split or non-split files */
> +    int (*bdrv_reopen_prepare)(BlockDriverState *bs,
> +                               BDRVReopenState **prs,
> +                               int flags);
> +    void (*bdrv_reopen_commit)(BlockDriverState *bs, BDRVReopenState *rs);
> +    void (*bdrv_reopen_abort)(BlockDriverState *bs, BDRVReopenState *rs);
> +
>      int (*bdrv_open)(BlockDriverState *bs, int flags);
>      int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int 
> flags);
>      int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
> @@ -335,6 +343,10 @@ struct BlockDriverState {
>      BlockJob *job;
>  };
>  
> +struct BDRVReopenState {
> +    BlockDriverState *bs;
> +};
> +
>  int get_tmp_filename(char *filename, int size);
>  
>  void bdrv_set_io_limits(BlockDriverState *bs,
> Index: qemu/block.h
> ===================================================================
> --- qemu.orig/block.h
> +++ qemu/block.h
> @@ -129,6 +129,10 @@ int bdrv_parse_cache_flags(const char *m
>  int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
>  int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
>                BlockDriver *drv);
> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
> +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int 
> flags);
> +void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs);
> +void bdrv_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs);
>  void bdrv_close(BlockDriverState *bs);
>  int bdrv_attach_dev(BlockDriverState *bs, void *dev);
>  void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev);
> Index: qemu/qemu-common.h
> ===================================================================
> --- qemu.orig/qemu-common.h
> +++ qemu/qemu-common.h
> @@ -223,6 +223,7 @@ typedef struct NICInfo NICInfo;
>  typedef struct HCIInfo HCIInfo;
>  typedef struct AudioState AudioState;
>  typedef struct BlockDriverState BlockDriverState;
> +typedef struct BDRVReopenState BDRVReopenState;
>  typedef struct DriveInfo DriveInfo;
>  typedef struct DisplayState DisplayState;
>  typedef struct DisplayChangeListener DisplayChangeListener;
> 
>