[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 resend 3/4] Block: readonly changes
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v2 resend 3/4] Block: readonly changes |
Date: |
Fri, 05 Feb 2010 09:20:12 +0100 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.5) Gecko/20091209 Fedora/3.0-4.fc12 Thunderbird/3.0 |
Am 04.02.2010 23:04, schrieb Naphtali Sprei:
> Open backing file for read-only
> During commit upgrade to read-write and back at end to read-only
>
> Signed-off-by: Naphtali Sprei <address@hidden>
> ---
> block.c | 68
> ++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> block_int.h | 1 +
> 2 files changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/block.c b/block.c
> index 66564de..4a9df91 100644
> --- a/block.c
> +++ b/block.c
> @@ -451,7 +451,6 @@ int bdrv_open2(BlockDriverState *bs, const char
> *filename, int flags,
> if (flags & (BDRV_O_CACHE_WB|BDRV_O_NOCACHE))
> bs->enable_write_cache = 1;
>
> - bs->read_only = (flags & BDRV_O_RDWR) == 0;
> if (!(flags & BDRV_O_FILE)) {
> open_flags = (flags & (BDRV_O_RDWR |
> BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
> if (bs->is_temporary) { /* snapshot should be writeable */
> @@ -466,6 +465,7 @@ int bdrv_open2(BlockDriverState *bs, const char
> *filename, int flags,
> goto free_and_fail;
> }
>
> + bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR);
> if (drv->bdrv_getlength) {
> bs->total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> }
> @@ -482,13 +482,28 @@ int bdrv_open2(BlockDriverState *bs, const char
> *filename, int flags,
> filename, bs->backing_file);
> if (bs->backing_format[0] != '\0')
> back_drv = bdrv_find_format(bs->backing_format);
> +
> + open_flags &= ~BDRV_O_RDWR; /* clear RW, then restore from orig */
> + if (bs->is_temporary) {
> + open_flags |= (flags & BDRV_O_RDWR);
> + }
> +
> ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
> back_drv);
> - bs->backing_hd->read_only = (open_flags & BDRV_O_RDWR) == 0;
> + if (ret < 0) {
> + open_flags &= ~BDRV_O_RDWR; /* Fall-back to read-only for the
> backing file */
> + ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
> + back_drv);
> + }
Why is this needed? The only case in which a backing file is opened
read-write is during commit, right? For commit there is certainly no use
in opening it read-only instead.
This whole code looks like there are cases where a backing file is still
opened read-write by default, though the commit message says that no
such backing files exist. Am I missing something?
> if (ret < 0) {
> bdrv_close(bs);
> return ret;
> }
> + if (!bs->is_temporary) {
> + bs->backing_hd->keep_read_only = bs->keep_read_only; /* base
> image inherits from "parent" and open read-only */
This looks like more than 80 characters on a line.
What would helps here and also would improve consistency in style is to
move the comments to the line before instead of sticking them at the end
of a code line. This is true even more if the comment actually applies
to a whole block and not only to the line in which it is written (you're
doing this in other places).
> + } else {
> + bs->backing_hd->keep_read_only = !(flags & BDRV_O_RDWR);
> + }
> }
>
> if (!bdrv_key_required(bs)) {
> @@ -564,19 +579,38 @@ int bdrv_commit(BlockDriverState *bs)
> {
> BlockDriver *drv = bs->drv;
> int64_t i, total_sectors;
> - int n, j;
> + int n, j, ro, open_flags;
> int ret = 0;
> unsigned char sector[512];
> + char filename[1024];
> + BlockDriverState *bs_rw, *bs_ro;
>
> if (!drv)
> return -ENOMEDIUM;
> +
> + if (!bs->backing_hd) {
> + return -ENOTSUP;
> + }
>
> - if (bs->read_only) {
> + if (bs->backing_hd->keep_read_only) {
> return -EACCES;
> }
> +
> + ro = bs->backing_hd->read_only;
> + strncpy(filename, bs->backing_hd->filename, sizeof(filename));
> + open_flags = bs->backing_hd->open_flags;
>
> - if (!bs->backing_hd) {
> - return -ENOTSUP;
> + if (ro) { /* re-open as RW */
> + bdrv_close(bs->backing_hd);
> + qemu_free(bs->backing_hd);
bdrv_delete is doing what you mean here. But actually, you don't need to
delete it, you can just reuse the old bs for re-opening the image.
> +
> + bs_rw = bdrv_new("");
> + ret = bdrv_open2(bs_rw, filename, open_flags | BDRV_O_RDWR, NULL);
> + if (ret < 0) {
> + bdrv_delete(bs_rw);
> + return -EACCES;
Why don't you pass the right return value up? Apart from that, you
should re-open the backing file (read-only) or the VM will get into
trouble...
> + }
> + bs->backing_hd = bs_rw;
Eek... ;-) Well, it should work, as far as I know the block drivers.
> }
>
> total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> @@ -584,11 +618,13 @@ int bdrv_commit(BlockDriverState *bs)
> if (drv->bdrv_is_allocated(bs, i, 65536, &n)) {
> for(j = 0; j < n; j++) {
> if (bdrv_read(bs, i, sector, 1) != 0) {
> - return -EIO;
> + ret = -EIO;
> + goto ro_cleanup;
> }
>
> if (bdrv_write(bs->backing_hd, i, sector, 1) != 0) {
> - return -EIO;
> + ret = -EIO;
> + goto ro_cleanup;
> }
> i++;
> }
> @@ -608,6 +644,22 @@ int bdrv_commit(BlockDriverState *bs)
> */
> if (bs->backing_hd)
> bdrv_flush(bs->backing_hd);
> +
> +ro_cleanup:
> +
> + if (ro) { /* re-open as RO */
> + bdrv_close(bs->backing_hd);
> + qemu_free(bs->backing_hd);
Again, I think bdrv_delete is needed.
> + bs_ro = bdrv_new("");
> + ret = bdrv_open2(bs_ro, filename, open_flags & ~BDRV_O_RDWR, NULL);
> + if (ret < 0) {
> + bdrv_delete(bs_ro);
> + return -EACCES;
Again, wrong return value.
> + }
> + bs->backing_hd = bs_ro;
> + bs->backing_hd->keep_read_only = 0;
> + }
> +
> return ret;
> }
>
> diff --git a/block_int.h b/block_int.h
> index 9144d37..02fae5b 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -130,6 +130,7 @@ struct BlockDriverState {
> int64_t total_sectors; /* if we are reading a disk image, give its
> size in sectors */
> int read_only; /* if true, the media is read only */
> + int keep_read_only; /* if true, the media was requested to stay read
> only */
> int open_flags; /* flags used to open the file */
> int removable; /* if true, the media can be removed */
> int locked; /* if true, the media cannot temporarily be ejected */
Kevin