[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [v8 Patch 6/6]Qemu: raw posix implementation of reopen
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [v8 Patch 6/6]Qemu: raw posix implementation of reopen functions |
Date: |
Fri, 04 Nov 2011 11:19:12 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20110927 Thunderbird/7.0 |
Am 30.10.2011 11:35, schrieb Supriya Kannery:
> raw-posix driver changes for bdrv_reopen_xx functions to
> safely reopen image files. Reopening of image files while
> changing hostcache dynamically, is handled here.
>
> Signed-off-by: Supriya Kannery <address@hidden>
>
> Index: qemu/block/raw.c
> ===================================================================
> --- qemu.orig/block/raw.c
> +++ qemu/block/raw.c
> @@ -9,6 +9,24 @@ static int raw_open(BlockDriverState *bs
> return 0;
> }
>
> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
> + int flags)
> +{
> + return bdrv_reopen_prepare(bs->file, prs, flags);
> +}
> +
> +static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs,
> + int flags)
> +{
> + bdrv_reopen_commit(bs->file, rs, flags);
> + bs->open_flags = bs->file->open_flags;
I think it should be bs->open_flags = flags, even if the underlying
driver masks something away (which it shouldn't do in the first place,
but using flags here is clearer).
Also I'm wondering if updating bs->open_flags isn't common to all
formats, so could we move it into the block.c function?
> +}
> +
> +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> + bdrv_reopen_abort(bs->file, rs);
> +}
> +
> static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t
> sector_num,
> int nb_sectors, QEMUIOVector *qiov)
> {
> @@ -107,7 +125,10 @@ static BlockDriver bdrv_raw = {
>
> /* It's really 0, but we need to make g_malloc() happy */
> .instance_size = 1,
> -
> + .bdrv_reopen_prepare
> + = raw_reopen_prepare,
> + .bdrv_reopen_commit = raw_reopen_commit,
> + .bdrv_reopen_abort = raw_reopen_abort,
> .bdrv_open = raw_open,
> .bdrv_close = raw_close,
Mostly a matter of taste, but I would prefer open/close to stay first
and having bdrv_reopen_* only after them.
>
> Index: qemu/block/raw-posix.c
> ===================================================================
> --- qemu.orig/block/raw-posix.c
> +++ qemu/block/raw-posix.c
> @@ -279,6 +279,60 @@ static int raw_open(BlockDriverState *bs
> return raw_open_common(bs, filename, flags, 0);
> }
>
> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
> + int flags)
> +{
> + BDRVReopenState *raw_rs = g_malloc0(sizeof(BDRVReopenState));
> + BDRVRawState *s = bs->opaque;
> +
> + raw_rs->bs = bs;
> + raw_rs->reopen_flags = s->open_flags;
> + raw_rs->reopen_fd = -1;
> + *prs = raw_rs;
> + int ret = 0;
> +
> + /* If only O_DIRECT to be toggled, use fcntl */
> + if (!((bs->open_flags & ~BDRV_O_NOCACHE) ^
> + (flags & ~BDRV_O_NOCACHE))) {
Wouldn't it be more readable like this?
/* Use fcntl if all affected flags can be changes this way */
fcntl_flags = BDRV_O_NOCACHE;
if ((bs->open_flags & ~fcntl_flags) == (flags & ~fcntl_flags)) {
...
> + raw_rs->reopen_fd = dup(s->fd);
> + if (raw_rs->reopen_fd <= 0) {
> + return -1;
> + }
> + if ((flags & BDRV_O_NOCACHE)) {
> + raw_rs->reopen_flags |= O_DIRECT;
> + } else {
> + raw_rs->reopen_flags &= ~O_DIRECT;
> + }
> + ret = fcntl_setfl(raw_rs->reopen_fd, raw_rs->reopen_flags);
> + }
I think there should be an else branch that returns an error. Currently
requests involving anything but BDRV_O_NOCACHE are completely ignored,
but still success is returned.
> +
> + /* TBD: Handle O_DSYNC and other flags */
> +
> + return ret;
> +}
Kevin
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [v8 Patch 6/6]Qemu: raw posix implementation of reopen functions,
Kevin Wolf <=