Am 11.10.2011 05:11, schrieb Supriya Kannery: > Struct BDRVReopenState introduced for handling reopen state of images. > This can be extended by each of the block drivers to reopen respective > image files. Implementation for raw-posix is done here. > > Signed-off-by: Supriya Kannery <supri...@linux.vnet.ibm.com>
Maybe it would make sense to split this into two patches, one for the block.c infrastructure and another one for adding the callbacks in drivers. > --- > block.c | 46 ++++++++++++++++++++++++++++++++-------- > block/raw-posix.c | 62 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > block_int.h | 15 +++++++++++++ > 3 files changed, 114 insertions(+), 9 deletions(-) > > Index: qemu/block/raw-posix.c > =================================================================== > --- qemu.orig/block/raw-posix.c > +++ qemu/block/raw-posix.c > @@ -279,6 +279,66 @@ static int raw_open(BlockDriverState *bs > return raw_open_common(bs, filename, flags, 0); > } > > +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **rs, > + int flags) > +{ > + BDRVReopenState *raw_rs = g_malloc0(sizeof(BDRVReopenState)); > + BDRVRawState *s = bs->opaque; > + > + raw_rs->bs = bs; > + raw_rs->reopen_flags = flags; > + raw_rs->reopen_fd = -1; > + > + /* If only O_DIRECT to be toggled, use fcntl */ > + if (!((bs->open_flags & ~BDRV_O_NOCACHE) ^ > + (flags & ~BDRV_O_NOCACHE))) { > + raw_rs->reopen_fd = dup(s->fd); > + if (raw_rs->reopen_fd <= 0) { > + return -1; This leaks raw_rs. > + } > + } > + > + /* TBD: Handle O_DSYNC and other flags */ > + *rs = raw_rs; > + return 0; > +} > + > +static int raw_reopen_commit(BDRVReopenState *rs) bdrv_reopen_commit must never fail. Any error that can happen must already be handled in bdrv_reopen_prepare. The commit function should really only do s->fd = rs->reopen_fd (besides cleanup), everything else should already be prepared. > +{ > + BlockDriverState *bs = rs->bs; > + BDRVRawState *s = bs->opaque; > + > + if (!rs->reopen_fd) { > + return -1; > + } > + > + int ret = fcntl_setfl(rs->reopen_fd, rs->reopen_flags); reopen_flags is BDRV_O_*, not O_*, so it needs to be translated. > + if (ret < 0) { > + return ret; > + } > + > + /* Set new flags, so replace old fd with new one */ > + close(s->fd); > + s->fd = rs->reopen_fd; > + s->open_flags = bs->open_flags = rs->reopen_flags; > + g_free(rs); > + rs = NULL; Setting to NULL has no effect, rs isn't read afterwards. > + > + return 0; > +} > + > +static int raw_reopen_abort(BDRVReopenState *rs) This must not fail either, so it can be void, too. > +{ > + if (rs->reopen_fd != -1) { > + close(rs->reopen_fd); > + rs->reopen_fd = -1; > + rs->reopen_flags = 0; > + } > + g_free(rs); > + rs = NULL; > + return 0; > +} > + > /* XXX: use host sector size if necessary with: > #ifdef DIOCGSECTORSIZE > { > @@ -896,6 +956,9 @@ static BlockDriver bdrv_file = { > .instance_size = sizeof(BDRVRawState), > .bdrv_probe = NULL, /* no probe for protocols */ > .bdrv_file_open = raw_open, > + .bdrv_reopen_prepare = raw_reopen_prepare, > + .bdrv_reopen_commit = raw_reopen_commit, > + .bdrv_reopen_abort = raw_reopen_abort, > .bdrv_read = raw_read, > .bdrv_write = raw_write, > .bdrv_close = raw_close, > @@ -1166,6 +1229,10 @@ static BlockDriver bdrv_host_device = { > .instance_size = sizeof(BDRVRawState), > .bdrv_probe_device = hdev_probe_device, > .bdrv_file_open = hdev_open, > + .bdrv_reopen_prepare > + = raw_reopen_prepare, > + .bdrv_reopen_commit = raw_reopen_commit, > + .bdrv_reopen_abort = raw_reopen_abort, > .bdrv_close = raw_close, > .bdrv_create = hdev_create, > .create_options = raw_create_options, > Index: qemu/block.c > =================================================================== > --- qemu.orig/block.c > +++ qemu/block.c > @@ -706,6 +706,7 @@ int bdrv_reopen(BlockDriverState *bs, in > { > BlockDriver *drv = bs->drv; > int ret = 0, open_flags; > + BDRVReopenState *rs; > > /* Quiesce IO for the given block device */ > qemu_aio_flush(); > @@ -713,20 +714,48 @@ 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_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 = drv->bdrv_reopen_prepare(bs, &rs, bdrv_flags); > if (ret < 0) { > - /* Reopen failed with orig and modified flags */ > - abort(); > + goto fail; > } > - } > + if (drv->bdrv_reopen_commit) { > + ret = drv->bdrv_reopen_commit(rs); > + if (ret < 0) { > + goto fail; > + } > + return 0; > + } Pull the return 0; out one level. It would be really strange if we turned a successful prepare into reopen_abort just because the driver doesn't need a commit function. (The other consistent way would be to require that if a driver implements one reopen function, it has to implement all three of them. I'm fine either way.) > +fail: > + if (drv->bdrv_reopen_abort) { > + ret = drv->bdrv_reopen_abort(rs); > + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); > + return ret; > + } > + > + } else { > + > + /* Use reopen procedure common for all drivers */ > + 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_REOPEN_FILE_FAILED, bs->filename); > + ret = bdrv_open(bs, bs->filename, open_flags, drv); > + if (ret < 0) { > + /* > + * Reopen with orig and modified flags failed > + */ > + abort(); Make this bs->drv = NULL, so that trying to access to image will fail, but at least not the whole VM crashes. > + } > + } > + } > return ret; > } > > Index: qemu/block_int.h > =================================================================== > --- qemu.orig/block_int.h > +++ qemu/block_int.h > @@ -55,6 +55,13 @@ struct BlockDriver { > int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char > *filename); > int (*bdrv_probe_device)(const char *filename); > int (*bdrv_open)(BlockDriverState *bs, int flags); > + > + /* For handling image reopen for split or non-split files */ > + int (*bdrv_reopen_prepare)(BlockDriverState *bs, BDRVReopenState **rs, > + int flags); > + int (*bdrv_reopen_commit)(BDRVReopenState *rs); > + int (*bdrv_reopen_abort)(BDRVReopenState *rs); > + > int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int > flags); > int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num, > uint8_t *buf, int nb_sectors); > @@ -207,6 +214,14 @@ struct BlockDriverState { > void *private; > }; > > +struct BDRVReopenState { > + BlockDriverState *bs; > + int reopen_flags; > + > + /* For raw-posix */ > + int reopen_fd; > +}; > + > struct BlockDriverAIOCB { > AIOPool *pool; > BlockDriverState *bs; > Index: qemu/qemu-common.h > =================================================================== > --- qemu.orig/qemu-common.h > +++ qemu/qemu-common.h > @@ -228,6 +228,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; > Index: qemu/block/raw.c > =================================================================== > --- qemu.orig/block/raw.c > +++ qemu/block/raw.c > @@ -9,6 +9,15 @@ static int raw_open(BlockDriverState *bs > return 0; > } > > +static int raw_reopen(BlockDriverState *bs, BDRVReopenState **prs, int flags) > +{ > + int ret = bdrv_reopen(bs->file, flags); > + if (!ret) { > + bs->open_flags = bs->file->open_flags; > + } > + return ret; > +} > + > static int raw_read(BlockDriverState *bs, int64_t sector_num, > uint8_t *buf, int nb_sectors) > { > @@ -128,6 +137,8 @@ static BlockDriver bdrv_raw = { > .instance_size = 1, > > .bdrv_open = raw_open, > + .bdrv_reopen_prepare > + = raw_reopen, > .bdrv_close = raw_close, > .bdrv_read = raw_read, > .bdrv_write = raw_write, I think raw must pass through all three functions. Otherwise it could happen that we need to abort, but the image has already been reopened. Kevin