Re: [Qemu-devel] [PATCH v2 resend 3/4] Block: readonly changes
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 > --- > 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++
[Qemu-devel] [PATCH v2 resend 3/4] Block: readonly changes
Open backing file for read-only During commit upgrade to read-write and back at end to read-only Signed-off-by: Naphtali Sprei --- 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); +} 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 */ +} 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); + +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; +} +bs->backing_hd = bs_rw; } 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); +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; +} +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 wa