[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for im
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen |
Date: |
Wed, 12 Oct 2011 16:55:15 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20110927 Thunderbird/7.0 |
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 <address@hidden>
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