Patchwork [v2,1/9] block: Framework for reopening image files safely

login
register
mail settings
Submitter Supriya Kannery
Date July 30, 2012, 9:34 p.m.
Message ID <20120730213422.21536.81486.sendpatchset@skannery.in.ibm.com>
Download mbox | patch
Permalink /patch/174096/
State New
Headers show

Comments

Supriya Kannery - July 30, 2012, 9:34 p.m.
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>

---
Stefan Hajnoczi - Aug. 1, 2012, 3:51 p.m.
On Mon, Jul 30, 2012 at 10:34 PM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret = 0;
> +    BDRVReopenState *reopen_state = 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;
> +    }
> +
> +    /* Use driver specific reopen() if available */
> +    if (drv->bdrv_reopen_prepare) {
> +        ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
> +         if (ret < 0) {

Indentation is off.

> +            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;
> +    }

This would be slightly simpler if written as:

if (drv->bdrv_reopen_prepare == NULL) {
    ...error return...
}

...success case...

Stefan
Luiz Capitulino - Aug. 2, 2012, 8:19 p.m.
On Tue, 31 Jul 2012 03:04:22 +0530
Supriya Kannery <supriyak@linux.vnet.ibm.com> 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>
> 
> ---
> 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;
> +
> +    /* Quiesce IO for the given block device */
> +    bdrv_drain_all();
> +    ret = bdrv_flush(bs);
> +    if (ret != 0) {
> +        error_set(errp, QERR_IO_ERROR);
> +        return;
> +    }
> +
> +    /* 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);

Why do you have to call bdrv_reopen_abort()? I'd expect bdrv_reopen_prepare()
(to be able) to undo anything it has done.

> +            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;
>
Jeff Cody - Aug. 8, 2012, 9:13 p.m.
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 <supriyak@linux.vnet.ibm.com>
> 
> ---
> 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;
> +
> +    /* Quiesce IO for the given block device */
> +    bdrv_drain_all();
> +    ret = bdrv_flush(bs);
> +    if (ret != 0) {
> +        error_set(errp, QERR_IO_ERROR);
> +        return;
> +    }
> +
> +    /* 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;

You also need to set bs->read_only here, like so:
     bs->read_only = !(bdrv_flags & BDRV_O_RDWR);


> +    } 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;
> 
>
Jeff Cody - Aug. 9, 2012, 4:26 a.m.
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 <supriyak@linux.vnet.ibm.com>
> 
> ---
> 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;
> 
>
Kevin Wolf - Aug. 9, 2012, 9:20 a.m.
Am 09.08.2012 06:26, schrieb 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 <supriyak@linux.vnet.ibm.com>
>>
>> ---
>> 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).

Doesn't this break the transactional semantics? I think you should only
prepare the bs->file reopen here and commit it when committing this one.

Kevin
Jeff Cody - Aug. 9, 2012, 1:02 p.m.
On 08/09/2012 05:20 AM, Kevin Wolf wrote:
> Am 09.08.2012 06:26, schrieb 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 <supriyak@linux.vnet.ibm.com>
>>>
>>> ---
>>> 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).
> 
> Doesn't this break the transactional semantics? I think you should only
> prepare the bs->file reopen here and commit it when committing this one.
> 

Yes, it would, so if everything stayed as it is now, that would be the
right way to do it... but one thing that would be nice (I also mention
this in my comments on patch 0/9) is that it become transactional for
multiple BlockDriverStates to be reopened.  That would obviously change
the interface a bit, so that multiple BDS entries could be queued, but
then it shouldn't be any different to queue the bs->file as well as the
bs.

All prepare() stages would happen on queued items, and only
progress to commit() if all prepare() stages passed, as you mentioned.

One other thing, and perhaps this is nit-picking some - but the
prepare() functions all modify the 'live' structures, after backing them
up into stashes.  So, on abort, the structures are restored from the
stashes, and on commit the stashes are discarded.  Conceptually, it
seems cleaner to this the opposite way - prepare() does it work into
temporary stashes, and the commit() then copies the data over from the
stash to the live structure, and abort() would just discard the stashes.
Supriya Kannery - Aug. 14, 2012, 8:54 a.m.
On 08/03/2012 01:49 AM, Luiz Capitulino wrote:
> On Tue, 31 Jul 2012 03:04:22 +0530
> Supriya Kannery<supriyak@linux.vnet.ibm.com>  wrote:
> 

>> +
>> +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;
>> +
>> +    /* Quiesce IO for the given block device */
>> +    bdrv_drain_all();
>> +    ret = bdrv_flush(bs);
>> +    if (ret != 0) {
>> +        error_set(errp, QERR_IO_ERROR);
>> +        return;
>> +    }
>> +
>> +    /* 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);
> 
> Why do you have to call bdrv_reopen_abort()? I'd expect bdrv_reopen_prepare()
> (to be able) to undo anything it has done.
> 
    Having separate abort function avoids cluttering of reopen-
prepare(). We wanted to logically separate out preparation, commit and 
abort. Same format is followed in implementations at block driver level
as well.
 -thanks, Supriya
Supriya Kannery - Aug. 14, 2012, 9:21 a.m.
On 08/09/2012 02:43 AM, Jeff Cody wrote:
> 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.
>>
>> +
>> +        bdrv_reopen_commit(bs, reopen_state);
>> +        bs->open_flags = bdrv_flags;
>
> You also need to set bs->read_only here, like so:
>       bs->read_only = !(bdrv_flags&  BDRV_O_RDWR);
>
>

Sure..will include in updated patch.

- thanks, Supriya
Supriya Kannery - Aug. 14, 2012, 10:24 a.m.
On 08/09/2012 06:32 PM, Jeff Cody wrote:
> On 08/09/2012 05:20 AM, Kevin Wolf wrote:
>> Am 09.08.2012 06:26, schrieb Jeff Cody:
>>> On 07/30/2012 05:34 PM, Supriya Kannery wrote:

>>>> +
>>>> +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).
>>
>> Doesn't this break the transactional semantics? I think you should only
>> prepare the bs->file reopen here and commit it when committing this one.
>>
> 
> Yes, it would, so if everything stayed as it is now, that would be the
> right way to do it... but one thing that would be nice (I also mention
> this in my comments on patch 0/9) is that it become transactional for
> multiple BlockDriverStates to be reopened.  That would obviously change
> the interface a bit, so that multiple BDS entries could be queued, but
> then it shouldn't be any different to queue the bs->file as well as the
> bs.
> 
> All prepare() stages would happen on queued items, and only
> progress to commit() if all prepare() stages passed, as you mentioned.

  Yes, will work on to get the transactional semantics applied to bs->file 
reopen as well.

> 
> One other thing, and perhaps this is nit-picking some - but the
> prepare() functions all modify the 'live' structures, after backing them
> up into stashes.  So, on abort, the structures are restored from the
> stashes, and on commit the stashes are discarded.  Conceptually, it
> seems cleaner to this the opposite way - prepare() does it work into
> temporary stashes, and the commit() then copies the data over from the
> stash to the live structure, and abort() would just discard the stashes.
> 
> 
  prepare()/commit() and abort() are from the perspective of new changes 
to be tried on respective block driver state. Once block driver is ready for the
state change, then commit() means we are commiting these new changes to driver
state. Similarly abort() means we are aborting these new changes half way
and getting back to old stashed state. This is the intended logic.

- thanks, Supriya

Patch

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