[Qemu-devel] Re: [PATCH] block: Fix bdrv_commit

[Naphtali Sprei]

Kevin Wolf wrote:
 When reopening the image, don't guess the driver, but use the same driver as
 was used before. This is important if the format=... option was used for that
 image.
 
 Signed-off-by: Kevin Wolf kw...@redhat.com
 ---
  block.c |6 +++---
  1 files changed, 3 insertions(+), 3 deletions(-)
 
 diff --git a/block.c b/block.c
 index adb3e5d..ffe82f8 100644
 --- a/block.c
 +++ b/block.c
 @@ -701,12 +701,12 @@ int bdrv_commit(BlockDriverState *bs)
  bdrv_delete(bs-backing_hd);
  bs-backing_hd = NULL;
  bs_rw = bdrv_new();
 -rw_ret = bdrv_open(bs_rw, filename, open_flags | BDRV_O_RDWR, NULL);
 +rw_ret = bdrv_open(bs_rw, filename, open_flags | BDRV_O_RDWR, drv);
  if (rw_ret  0) {
  bdrv_delete(bs_rw);
  /* try to re-open read-only */
  bs_ro = bdrv_new();
 -ret = bdrv_open(bs_ro, filename, open_flags  ~BDRV_O_RDWR, 
 NULL);
 +ret = bdrv_open(bs_ro, filename, open_flags  ~BDRV_O_RDWR, drv);
  if (ret  0) {
  bdrv_delete(bs_ro);
  /* drive not functional anymore */
 @@ -758,7 +758,7 @@ ro_cleanup:
  bdrv_delete(bs-backing_hd);
  bs-backing_hd = NULL;
  bs_ro = bdrv_new();
 -ret = bdrv_open(bs_ro, filename, open_flags  ~BDRV_O_RDWR, NULL);
 +ret = bdrv_open(bs_ro, filename, open_flags  ~BDRV_O_RDWR, drv);
  if (ret  0) {
  bdrv_delete(bs_ro);
  /* drive not functional anymore */

Acked-by: Naphtali Sprei nsp...@redhat.com

[Qemu-devel] [PATCH] block: read-only: open cdrom as read-only when using monitor's change command

[Naphtali Sprei]

Current code of monitor command: 'change', used to open file for read-write
uncoditionally. Change to open it as read-only for CDROM, and read-write for 
all others.

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 monitor.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index c25d551..df5a15d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1083,6 +1083,7 @@ static int do_change_block(Monitor *mon, const char 
*device,
 {
 BlockDriverState *bs;
 BlockDriver *drv = NULL;
+int bdrv_flags;
 
 bs = bdrv_find(device);
 if (!bs) {
@@ -1099,7 +1100,8 @@ static int do_change_block(Monitor *mon, const char 
*device,
 if (eject_device(mon, bs, 0)  0) {
 return -1;
 }
-if (bdrv_open(bs, filename, BDRV_O_RDWR, drv)  0) {
+bdrv_flags = bdrv_get_type_hint(bs) == BDRV_TYPE_CDROM ? 0 : BDRV_O_RDWR;
+if (bdrv_open(bs, filename, bdrv_flags, drv)  0) {
 qerror_report(QERR_OPEN_FILE_FAILED, filename);
 return -1;
 }
-- 
1.6.3.3

[Qemu-devel] [PATCH v2] read-only: allow read-only CDROM with any interface

[Naphtali Sprei]

v1 - v2
cosmetic change of if block arrangement

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 vl.c |   13 +
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/vl.c b/vl.c
index d69250c..11c68f2 100644
--- a/vl.c
+++ b/vl.c
@@ -1222,19 +1222,16 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
 bdrv_flags = ~BDRV_O_NATIVE_AIO;
 }
 
-if (ro == 1) {
+if (media == MEDIA_CDROM) {
+/* CDROM is fine for any interface, don't check.  */
+ro = 1;
+} else if (ro == 1) {
 if (type != IF_SCSI  type != IF_VIRTIO  type != IF_FLOPPY) {
 fprintf(stderr, qemu: readonly flag not supported for drive with 
this interface\n);
 return NULL;
 }
 }
-/* 
- * cdrom is read-only. Set it now, after above interface checking
- * since readonly attribute not explicitly required, so no error.
- */
-if (media == MEDIA_CDROM) {
-ro = 1;
-}
+
 bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
 
 if (bdrv_open2(dinfo-bdrv, file, bdrv_flags, drv)  0) {
-- 
1.6.3.3

[Qemu-devel] [PATCH] read-only: allow read-only CDROM with any interface

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 vl.c |   13 +
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/vl.c b/vl.c
index 2e38b77..d3863d7 100644
--- a/vl.c
+++ b/vl.c
@@ -1255,19 +1255,16 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
 bdrv_flags = ~BDRV_O_NATIVE_AIO;
 }
 
-if (ro == 1) {
+if (media == MEDIA_CDROM) {
+ro = 1;
+}
+if (ro == 1 
+media != MEDIA_CDROM) { /* CDROM is fine for any interface, don't 
check */
 if (type != IF_SCSI  type != IF_VIRTIO  type != IF_FLOPPY) {
 fprintf(stderr, qemu: readonly flag not supported for drive with 
this interface\n);
 return NULL;
 }
 }
-/* 
- * cdrom is read-only. Set it now, after above interface checking
- * since readonly attribute not explicitly required, so no error.
- */
-if (media == MEDIA_CDROM) {
-ro = 1;
-}
 bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
 
 if (bdrv_open2(dinfo-bdrv, file, bdrv_flags, drv)  0) {
-- 
1.6.3.3

[Qemu-devel] [PATCH v2 1/2] read-only: minor cleanup

[Naphtali Sprei]

Really use read-only flags for opening the file when asked for read-only

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 qemu-nbd.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index eac0c21..a393583 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -258,6 +258,7 @@ int main(int argc, char **argv)
 break;
 case 'r':
 readonly = true;
+flags = ~BDRV_O_RDWR;
 break;
 case 'P':
 partition = strtol(optarg, end, 0);
-- 
1.6.3.3

[Qemu-devel] [PATCH 1/2] read-only: minor cleanup

[Naphtali Sprei]

Really use read-only flags for opening the file when asked for read-only

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 qemu-nbd.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index eac0c21..d803bfd 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -258,6 +258,7 @@ int main(int argc, char **argv)
 break;
 case 'r':
 readonly = true;
+flags = BDRV_O_RDWR;
 break;
 case 'P':
 partition = strtol(optarg, end, 0);
-- 
1.6.3.3

[Qemu-devel] [PATCH 2/2] read-only: Another minor cleanup

[Naphtali Sprei]

Don't rely on CDROM hint for read_only attribute

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 hw/scsi-disk.c |3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 8f7ffc1..5cd6ae6 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -612,8 +612,7 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, 
uint8_t *outbuf)
 
 p[1] = 0; /* Default media type.  */
 p[3] = 0; /* Block descriptor length.  */
-if (bdrv_get_type_hint(s-bs) == BDRV_TYPE_CDROM ||
-bdrv_is_read_only(s-bs)) {
+if (bdrv_is_read_only(s-bs)) {
 p[2] = 0x80; /* Readonly.  */
 }
 p += 4;
-- 
1.6.3.3

Re: [Qemu-devel] [BUG] Regression: readonly raw images no longer work

[Naphtali Sprei]

Stefan Weil wrote:
 This command used to work, but fails now:
 
 $ i386-softmmu/qemu -snapshot /dev/sda
 qemu: could not open disk image /dev/sda: Permission denied
 
 $ ls -l /dev/sda
 brw-rw-r-- 1 root disk 8, 0 13. Feb 08:55 /dev/sda
 
 The original file of a snapshot needs only read access,
 but QEMU tries read/write access and fails.
 
 Variants of above command using -hda or -drive
 also fail with the same error message.
 
 I did not test whether the regression affects other
 kinds of images, too. Maybe only raw images trigger
 no longer work.
 
 Regards
 Stefan Weil
 
 
 

Sorry for the late reply,
this is my fault. It will fail for any image format.

It's already fixed with commit 4dca4b639cb20fee38f6eec0a391aecc0ad8848d :
block: more read-only changes, related to backing files

Now, since the /dev/sda is the backing-file, it's opened as read-only, no 
permission problem.

Notice that if you try to commit your changes to the read-only disk (in monitor 
command), 
it will be silently ignored, no error or warning displayed.
Should I add a warning/error printing ? 

 Naphtali

[Qemu-devel] [PATCH v4 rebased] block: more read-only changes, related to backing files

[Naphtali Sprei]

Open backing file read-only where possible
Upgrade backing file to read-write during commit, back to read-only after commit
  If upgrade fail, back to read-only. If also fail, disconnect the drive.

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |   83 ++
 block_int.h |2 +
 2 files changed, 73 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index af56ea7..31d1ba4 100644
--- a/block.c
+++ b/block.c
@@ -363,6 +363,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 bs-is_temporary = 0;
 bs-encrypted = 0;
 bs-valid_key = 0;
+bs-open_flags = flags;
 /* buffer_alignment defaulted to 512, drivers can change this value */
 bs-buffer_alignment = 512;
 
@@ -450,8 +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;
-
 /*
  * Clear flags that are internal to the block layer before opening the
  * image.
@@ -472,6 +471,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;
 }
@@ -488,13 +488,22 @@ 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);
+
+/* backing files always opened read-only */
+open_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) {
 bdrv_close(bs);
 return ret;
 }
+if (bs-is_temporary) {
+bs-backing_hd-keep_read_only = !(flags  BDRV_O_RDWR);
+} else {
+/* base image inherits from parent */
+bs-backing_hd-keep_read_only = bs-keep_read_only;
+}
 }
 
 if (!bdrv_key_required(bs)) {
@@ -570,19 +579,48 @@ int bdrv_commit(BlockDriverState *bs)
 {
 BlockDriver *drv = bs-drv;
 int64_t i, total_sectors;
-int n, j;
-int ret = 0;
+int n, j, ro, open_flags;
+int ret = 0, rw_ret = 0;
 unsigned char sector[512];
+char filename[1024];
+BlockDriverState *bs_rw, *bs_ro;
 
 if (!drv)
 return -ENOMEDIUM;
-
-if (bs-read_only) {
-   return -EACCES;
+
+if (!bs-backing_hd) {
+return -ENOTSUP;
 }
 
-if (!bs-backing_hd) {
-   return -ENOTSUP;
+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 (ro) {
+/* re-open as RW */
+bdrv_delete(bs-backing_hd);
+bs-backing_hd = NULL;
+bs_rw = bdrv_new();
+rw_ret = bdrv_open2(bs_rw, filename, open_flags | BDRV_O_RDWR, NULL);
+if (rw_ret  0) {
+bdrv_delete(bs_rw);
+/* try to re-open read-only */
+bs_ro = bdrv_new();
+ret = bdrv_open2(bs_ro, filename, open_flags  ~BDRV_O_RDWR, NULL);
+if (ret  0) {
+bdrv_delete(bs_ro);
+/* drive not functional anymore */
+bs-drv = NULL;
+return ret;
+}
+bs-backing_hd = bs_ro;
+return rw_ret;
+}
+bs-backing_hd = bs_rw;
 }
 
 total_sectors = bdrv_getlength(bs)  BDRV_SECTOR_BITS;
@@ -590,11 +628,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++;
}
@@ -614,6 +654,25 @@ int bdrv_commit(BlockDriverState *bs)
  */
 if (bs-backing_hd)
 bdrv_flush(bs-backing_hd);
+
+ro_cleanup:
+
+if (ro) {
+/* re-open as RO */
+bdrv_delete(bs-backing_hd);
+bs-backing_hd = NULL;
+bs_ro = bdrv_new();
+ret = bdrv_open2(bs_ro, filename, open_flags  ~BDRV_O_RDWR, NULL);
+if (ret  0) {
+bdrv_delete(bs_ro);
+/* drive not functional anymore */
+bs-drv = NULL;
+return ret;
+}
+bs

Re: [Qemu-devel] [PATCH v2] qemu-img: Fix qemu-img can't create qcow image based on read-only image

[Naphtali Sprei]

Anthony Liguori wrote:
 On 01/28/2010 08:15 PM, Sheng Yang wrote:
 Commit 03cbdac7 Disable fall-back to read-only when cannot open drive's
 file for read-write result in read-only image can't be used as backed
 image in qemu-img.

 Cc: Naphtali Spreinsp...@redhat.com
 Signed-off-by: Sheng Yangsh...@linux.intel.com

 
 Applied.  Thanks.

I've sent a patch that includes this patch, too ( 
http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg00578.html )
Should I rebase my patch ?

  Naphtali


 
 Regards,
 
 Anthony Liguori

[Qemu-devel] [PATCH v4] block: more read-only changes, related to backing files

[Naphtali Sprei]

This version includes handling the case the backing file cannot be re-opened


Open backing file read-only where possible
Upgrade backing file to read-write during commit, back to read-only after commit
  If upgrade fail, back to read-only. If also fail, disconnect the drive.
Added option for qemu-img.c bdrv_new_open() to open file as read-only

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |   80 +++---
 block_int.h |2 +
 qemu-img.c  |   15 +++---
 3 files changed, 82 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index 1919d19..612a24b 100644
--- a/block.c
+++ b/block.c
@@ -363,6 +363,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 bs-is_temporary = 0;
 bs-encrypted = 0;
 bs-valid_key = 0;
+bs-open_flags = flags;
 /* buffer_alignment defaulted to 512, drivers can change this value */
 bs-buffer_alignment = 512;
 
@@ -450,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 */
@@ -465,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;
 }
@@ -481,13 +482,22 @@ 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);
+
+/* backing files always opened read-only */
+open_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) {
 bdrv_close(bs);
 return ret;
 }
+if (bs-is_temporary) {
+bs-backing_hd-keep_read_only = !(flags  BDRV_O_RDWR);
+} else {
+/* base image inherits from parent */
+bs-backing_hd-keep_read_only = bs-keep_read_only;
+}
 }
 
 if (!bdrv_key_required(bs)) {
@@ -563,19 +573,48 @@ int bdrv_commit(BlockDriverState *bs)
 {
 BlockDriver *drv = bs-drv;
 int64_t i, total_sectors;
-int n, j;
-int ret = 0;
+int n, j, ro, open_flags;
+int ret = 0, rw_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;
 }
-
-if (!bs-backing_hd) {
-   return -ENOTSUP;
+
+ro = bs-backing_hd-read_only;
+strncpy(filename, bs-backing_hd-filename, sizeof(filename));
+open_flags =  bs-backing_hd-open_flags;
+
+if (ro) {
+/* re-open as RW */
+bdrv_delete(bs-backing_hd);
+bs-backing_hd = NULL;
+bs_rw = bdrv_new();
+rw_ret = bdrv_open2(bs_rw, filename, open_flags | BDRV_O_RDWR, NULL);
+if (rw_ret  0) {
+bdrv_delete(bs_rw);
+/* try to re-open read-only */
+bs_ro = bdrv_new();
+ret = bdrv_open2(bs_ro, filename, open_flags  ~BDRV_O_RDWR, NULL);
+if (ret  0) {
+bdrv_delete(bs_ro);
+/* drive not functional anymore */
+bs-drv = NULL;
+return ret;
+}
+bs-backing_hd = bs_ro;
+return rw_ret;
+}
+bs-backing_hd = bs_rw;
 }
 
 total_sectors = bdrv_getlength(bs)  BDRV_SECTOR_BITS;
@@ -583,11 +622,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++;
}
@@ -607,6 +648,25 @@ int bdrv_commit(BlockDriverState *bs)
  */
 if (bs-backing_hd)
 bdrv_flush(bs-backing_hd);
+
+ro_cleanup:
+
+if (ro) {
+/* re-open as RO */
+bdrv_delete(bs-backing_hd);
+bs-backing_hd = NULL;
+bs_ro = bdrv_new();
+ret = bdrv_open2

[Qemu-devel] [PATCH v3] block: more read-only changes, related to backing files

[Naphtali Sprei]

This version addresses comments by Kevin Wolf kw...@redhat.com to v2
Also separate commits squashed.


Open image file read-only where possible
Upgrade file to read-write during commit, back to read-only after commit
Added option for qemu-img.c bdrv_new_open() to open file as read-only

qemu-img changes based on patch by Sheng Yang sh...@linux.intel.com


Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |   75 +++
 block_int.h |2 +
 qemu-img.c  |   15 
 3 files changed, 77 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index 1919d19..be5f000 100644
--- a/block.c
+++ b/block.c
@@ -363,6 +363,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 bs-is_temporary = 0;
 bs-encrypted = 0;
 bs-valid_key = 0;
+bs-open_flags = flags;
 /* buffer_alignment defaulted to 512, drivers can change this value */
 bs-buffer_alignment = 512;
 
@@ -450,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 */
@@ -465,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;
 }
@@ -481,13 +482,21 @@ 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;
+
 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) {
 bdrv_close(bs);
 return ret;
 }
+if (!bs-is_temporary) {
+/* base image inherits from parent and open read-only */
+bs-backing_hd-keep_read_only = bs-keep_read_only;
+} else {
+bs-backing_hd-keep_read_only = !(flags  BDRV_O_RDWR);
+}
 }
 
 if (!bdrv_key_required(bs)) {
@@ -563,19 +572,46 @@ int bdrv_commit(BlockDriverState *bs)
 {
 BlockDriver *drv = bs-drv;
 int64_t i, total_sectors;
-int n, j;
-int ret = 0;
+int n, j, ro, open_flags;
+int ret = 0, rw_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;
 }
-
-if (!bs-backing_hd) {
-   return -ENOTSUP;
+
+ro = bs-backing_hd-read_only;
+strncpy(filename, bs-backing_hd-filename, sizeof(filename));
+open_flags =  bs-backing_hd-open_flags;
+
+if (ro) {
+/* re-open as RW */
+bdrv_delete(bs-backing_hd);
+
+bs_rw = bdrv_new();
+rw_ret = bdrv_open2(bs_rw, filename, open_flags | BDRV_O_RDWR, NULL);
+if (rw_ret  0) {
+bdrv_delete(bs_rw);
+/* try to re-open read-only */
+bs_ro = bdrv_new();
+ret = bdrv_open2(bs_ro, filename, open_flags  ~BDRV_O_RDWR, NULL);
+if (ret  0) {
+bdrv_delete(bs_ro);
+return ret;
+}
+bs-backing_hd = bs_ro;
+return rw_ret;
+}
+bs-backing_hd = bs_rw;
 }
 
 total_sectors = bdrv_getlength(bs)  BDRV_SECTOR_BITS;
@@ -583,11 +619,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++;
}
@@ -607,6 +645,23 @@ int bdrv_commit(BlockDriverState *bs)
  */
 if (bs-backing_hd)
 bdrv_flush(bs-backing_hd);
+
+ro_cleanup:
+
+if (ro) {
+/* re-open as RO */
+bdrv_delete(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 ret;
+}
+bs

Re: [Qemu-devel] [PATCH 3/4] Block: readonly changes

[Naphtali Sprei]

Jamie Lokier wrote:
 Naphtali Sprei wrote:
 Open backing file for read-only
 During commit upgrade to read-write and back at end to read-only
 
 +if (ro) { /* re-open as RO */
 +bs_ro = bdrv_new();
 +ret = bdrv_open2(bs_ro, bs-backing_hd-filename,  
 bs-backing_hd-open_flags  ~BDRV_O_RDWR, NULL);
 +if (ret  0) {
 +bdrv_delete(bs_ro);
 +return -EACCES;
 +}
 +bdrv_close(bs-backing_hd);
 +qemu_free(bs-backing_hd);
 +bs-backing_hd = bs_ro;
 +bs-backing_hd-keep_read_only = 0;
 +}
 
 I think the general idea is perfect.
 
 A couple of concerns come to mind.
 
 1. When changing read-write to read-only, if the backing file is a complex
format like qcow2 (or any others), is it possible for this bdrv_open2()
to read metadata such as format indexes, and even data, _before_
all changes maintained by bs-backing_hd have been written to the file?
 
(If the complex formats were like real filesystems and had a mounted
flags as real filesystems tend to, then it would be an issue, but I'm
not aware of any of them doing that.)
 
Are there any such issues when switching from read-only to read-write
earlier?  (It seems unlikely).
 

Good question. I looked at some of the formats (qcow, qcow2, vmdk) and didn't 
see
anything problematic, since in the close function I didn't see any changes to 
the real file,
only in-memory data and memory free.
But an answer from an expert would help.

 2. Secondly, what if the re-open gets a different file (testable with
fstat()).  I know, you get what you deserve if you rename files, but
still, do any of the formats which use backing files have a UUID check
or something to confirm they are using the right backing file, which
might be subverted by this?

I didn't see any such checking/validation.
It seems that handling such cases will complicate things more than you gain.

 
 3. What about the bdrv file/device-locking which was actively looking
like it might get in a couple of months ago.  Did it get in, and if
so does it conflict with this upgrade pattern?

AFAIK, the locks thread terminated, don't think anything committed.
But surely, there's a tight relationship between read-only/locks and sharing.

 
 Thanks!
 -- Jamie

 Thanks,

  Naphtali

[Qemu-devel] [PATCH v2 2/4] qemu-img: Fix qemu-img can't create qcow image based on read-only image

[Naphtali Sprei]

Open image file read-only where possible
Patch originally written by Sheng Yang sh...@linux.intel.com

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 qemu-img.c |   15 ++-
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index cbba4fc..b0ac9eb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -188,11 +188,13 @@ static int read_password(char *buf, int buf_size)
 #endif
 
 static BlockDriverState *bdrv_new_open(const char *filename,
-   const char *fmt)
+   const char *fmt,
+   int readonly)
 {
 BlockDriverState *bs;
 BlockDriver *drv;
 char password[256];
+int flags = BRDV_O_FLAGS;
 
 bs = bdrv_new();
 if (!bs)
@@ -204,7 +206,10 @@ static BlockDriverState *bdrv_new_open(const char 
*filename,
 } else {
 drv = NULL;
 }
-if (bdrv_open2(bs, filename, BRDV_O_FLAGS | BDRV_O_RDWR, drv)  0) {
+if (!readonly) {
+flags |= BDRV_O_RDWR;
+}
+if (bdrv_open2(bs, filename, flags, drv)  0) {
 error(Could not open '%s', filename);
 }
 if (bdrv_is_encrypted(bs)) {
@@ -343,7 +348,7 @@ static int img_create(int argc, char **argv)
 }
 }
 
-bs = bdrv_new_open(backing_file-value.s, fmt);
+bs = bdrv_new_open(backing_file-value.s, fmt, 1);
 bdrv_get_geometry(bs, size);
 size *= 512;
 bdrv_delete(bs);
@@ -627,7 +632,7 @@ static int img_convert(int argc, char **argv)
 
 total_sectors = 0;
 for (bs_i = 0; bs_i  bs_n; bs_i++) {
-bs[bs_i] = bdrv_new_open(argv[optind + bs_i], fmt);
+bs[bs_i] = bdrv_new_open(argv[optind + bs_i], fmt, 1);
 if (!bs[bs_i])
 error(Could not open '%s', argv[optind + bs_i]);
 bdrv_get_geometry(bs[bs_i], bs_sectors);
@@ -685,7 +690,7 @@ static int img_convert(int argc, char **argv)
 }
 }
 
-out_bs = bdrv_new_open(out_filename, out_fmt);
+out_bs = bdrv_new_open(out_filename, out_fmt, 0);
 
 bs_i = 0;
 bs_offset = 0;
-- 
1.6.3.3

[Qemu-devel] [PATCH v2 0/4] block: more read-only changes, related to backing files

[Naphtali Sprei]

This is version 2. The change between previous patch (only 3/4) is the order of 
closing/re-opening the image.

The read-only changes broke qemu-img create with read-only base image
These changes fix it.
It also make things a little safer, opening the backing file for read-only,
upgrading to read-write only for commit, and back to read-only when done.

Naphtali Sprei (4):
  Add open_flags to BlockDriverState Will be used later
  qemu-img: Fix qemu-img can't create qcow image based on read-only
image
  Block: readonly changes
  Open backing file read-only also for snapshot mode

[Qemu-devel] [PATCH v2 1/4] Add open_flags to BlockDriverState Will be used later

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |1 +
 block_int.h |1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 1919d19..66564de 100644
--- a/block.c
+++ b/block.c
@@ -363,6 +363,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 bs-is_temporary = 0;
 bs-encrypted = 0;
 bs-valid_key = 0;
+bs-open_flags = flags;
 /* buffer_alignment defaulted to 512, drivers can change this value */
 bs-buffer_alignment = 512;
 
diff --git a/block_int.h b/block_int.h
index a0ebd90..9144d37 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 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 */
 int encrypted; /* if true, the media is encrypted */
-- 
1.6.3.3

[Qemu-devel] [PATCH v2 3/4] Block: readonly changes

[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 nsp...@redhat.com
---
 block.c |   64 +++---
 block_int.h |1 +
 2 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 66564de..527b146 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,34 @@ int bdrv_commit(BlockDriverState *bs)
 {
 BlockDriver *drv = bs-drv;
 int64_t i, total_sectors;
-int n, j;
+int n, j, ro;
 int ret = 0;
 unsigned char sector[512];
+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;
 
-if (!bs-backing_hd) {
-   return -ENOTSUP;
+if (ro) { /* re-open as RW */
+bs_rw = bdrv_new();
+ret = bdrv_open2(bs_rw, bs-backing_hd-filename,  
bs-backing_hd-open_flags | BDRV_O_RDWR, NULL);
+if (ret  0) {
+bdrv_delete(bs_rw);
+return -EACCES;
+}
+bdrv_close(bs-backing_hd);
+qemu_free(bs-backing_hd);
+bs-backing_hd = bs_rw;
 }
 
 total_sectors = bdrv_getlength(bs)  BDRV_SECTOR_BITS;
@@ -584,11 +614,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 +640,22 @@ int bdrv_commit(BlockDriverState *bs)
  */
 if (bs-backing_hd)
 bdrv_flush(bs-backing_hd);
+
+ro_cleanup:
+
+if (ro) { /* re-open as RO */
+bs_ro = bdrv_new();
+ret = bdrv_open2(bs_ro, bs-backing_hd-filename,  
bs-backing_hd-open_flags  ~BDRV_O_RDWR, NULL);
+if (ret  0) {
+bdrv_delete(bs_ro);
+return -EACCES;
+}
+bdrv_close(bs-backing_hd);
+qemu_free(bs-backing_hd);
+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

[Qemu-devel] [PATCH v2 4/4] Open backing file read-only also for snapshot mode

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |   10 +-
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 527b146..1db9961 100644
--- a/block.c
+++ b/block.c
@@ -483,19 +483,11 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 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);
-}
+open_flags = ~BDRV_O_RDWR;
 
 ret = bdrv_open2(bs-backing_hd, backing_filename, open_flags,
  back_drv);
 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;
 }
-- 
1.6.3.3

[Qemu-devel] Re: [PATCH v2 0/4] block: more read-only changes, related to backing files

[Naphtali Sprei]

Naphtali Sprei wrote:
 This is version 2. The change between previous patch (only 3/4) is the order 
 of closing/re-opening the image.

Sorry,
patch sent without the change of version 2 (in 3/4).
Will send later.

  Naphtali

[Qemu-devel] [PATCH v2 2/4] qemu-img: Fix qemu-img can't create qcow image based on read-only image

[Naphtali Sprei]

Open image file read-only where possible
Patch originally written by Sheng Yang sh...@linux.intel.com

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 qemu-img.c |   15 ++-
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index cbba4fc..b0ac9eb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -188,11 +188,13 @@ static int read_password(char *buf, int buf_size)
 #endif
 
 static BlockDriverState *bdrv_new_open(const char *filename,
-   const char *fmt)
+   const char *fmt,
+   int readonly)
 {
 BlockDriverState *bs;
 BlockDriver *drv;
 char password[256];
+int flags = BRDV_O_FLAGS;
 
 bs = bdrv_new();
 if (!bs)
@@ -204,7 +206,10 @@ static BlockDriverState *bdrv_new_open(const char 
*filename,
 } else {
 drv = NULL;
 }
-if (bdrv_open2(bs, filename, BRDV_O_FLAGS | BDRV_O_RDWR, drv)  0) {
+if (!readonly) {
+flags |= BDRV_O_RDWR;
+}
+if (bdrv_open2(bs, filename, flags, drv)  0) {
 error(Could not open '%s', filename);
 }
 if (bdrv_is_encrypted(bs)) {
@@ -343,7 +348,7 @@ static int img_create(int argc, char **argv)
 }
 }
 
-bs = bdrv_new_open(backing_file-value.s, fmt);
+bs = bdrv_new_open(backing_file-value.s, fmt, 1);
 bdrv_get_geometry(bs, size);
 size *= 512;
 bdrv_delete(bs);
@@ -627,7 +632,7 @@ static int img_convert(int argc, char **argv)
 
 total_sectors = 0;
 for (bs_i = 0; bs_i  bs_n; bs_i++) {
-bs[bs_i] = bdrv_new_open(argv[optind + bs_i], fmt);
+bs[bs_i] = bdrv_new_open(argv[optind + bs_i], fmt, 1);
 if (!bs[bs_i])
 error(Could not open '%s', argv[optind + bs_i]);
 bdrv_get_geometry(bs[bs_i], bs_sectors);
@@ -685,7 +690,7 @@ static int img_convert(int argc, char **argv)
 }
 }
 
-out_bs = bdrv_new_open(out_filename, out_fmt);
+out_bs = bdrv_new_open(out_filename, out_fmt, 0);
 
 bs_i = 0;
 bs_offset = 0;
-- 
1.6.3.3

[Qemu-devel] [PATCH v2 resend 0/4] block: more read-only changes, related to backing files

[Naphtali Sprei]

This is version 2. The change between previous patch (only 3/4) is the order of 
closing/re-opening the image.

Naphtali Sprei (4):
  Add open_flags to BlockDriverState Will be used later
  qemu-img: Fix qemu-img can't create qcow image based on read-only
image
  Block: readonly changes
  Open backing file read-only also for snapshot mode

[Qemu-devel] [PATCH v2 resend 1/4] Add open_flags to BlockDriverState Will be used later

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |1 +
 block_int.h |1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 1919d19..66564de 100644
--- a/block.c
+++ b/block.c
@@ -363,6 +363,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 bs-is_temporary = 0;
 bs-encrypted = 0;
 bs-valid_key = 0;
+bs-open_flags = flags;
 /* buffer_alignment defaulted to 512, drivers can change this value */
 bs-buffer_alignment = 512;
 
diff --git a/block_int.h b/block_int.h
index a0ebd90..9144d37 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 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 */
 int encrypted; /* if true, the media is encrypted */
-- 
1.6.3.3

[Qemu-devel] [PATCH v2 resend 4/4] Open backing file read-only also for snapshot mode

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |   10 +-
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 4a9df91..780cea9 100644
--- a/block.c
+++ b/block.c
@@ -483,19 +483,11 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 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);
-}
+open_flags = ~BDRV_O_RDWR;
 
 ret = bdrv_open2(bs-backing_hd, backing_filename, open_flags,
  back_drv);
 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;
 }
-- 
1.6.3.3

[Qemu-devel] [PATCH v2 resend 3/4] Block: readonly changes

[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 nsp...@redhat.com
---
 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 was requested to stay read only 
*/
 int open_flags

[Qemu-devel] [PATCH v2 resend 2/4] qemu-img: Fix qemu-img can't create qcow image based on read-only image

[Naphtali Sprei]

Open image file read-only where possible
Patch originally written by Sheng Yang sh...@linux.intel.com

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 qemu-img.c |   15 ++-
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index cbba4fc..b0ac9eb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -188,11 +188,13 @@ static int read_password(char *buf, int buf_size)
 #endif
 
 static BlockDriverState *bdrv_new_open(const char *filename,
-   const char *fmt)
+   const char *fmt,
+   int readonly)
 {
 BlockDriverState *bs;
 BlockDriver *drv;
 char password[256];
+int flags = BRDV_O_FLAGS;
 
 bs = bdrv_new();
 if (!bs)
@@ -204,7 +206,10 @@ static BlockDriverState *bdrv_new_open(const char 
*filename,
 } else {
 drv = NULL;
 }
-if (bdrv_open2(bs, filename, BRDV_O_FLAGS | BDRV_O_RDWR, drv)  0) {
+if (!readonly) {
+flags |= BDRV_O_RDWR;
+}
+if (bdrv_open2(bs, filename, flags, drv)  0) {
 error(Could not open '%s', filename);
 }
 if (bdrv_is_encrypted(bs)) {
@@ -343,7 +348,7 @@ static int img_create(int argc, char **argv)
 }
 }
 
-bs = bdrv_new_open(backing_file-value.s, fmt);
+bs = bdrv_new_open(backing_file-value.s, fmt, 1);
 bdrv_get_geometry(bs, size);
 size *= 512;
 bdrv_delete(bs);
@@ -627,7 +632,7 @@ static int img_convert(int argc, char **argv)
 
 total_sectors = 0;
 for (bs_i = 0; bs_i  bs_n; bs_i++) {
-bs[bs_i] = bdrv_new_open(argv[optind + bs_i], fmt);
+bs[bs_i] = bdrv_new_open(argv[optind + bs_i], fmt, 1);
 if (!bs[bs_i])
 error(Could not open '%s', argv[optind + bs_i]);
 bdrv_get_geometry(bs[bs_i], bs_sectors);
@@ -685,7 +690,7 @@ static int img_convert(int argc, char **argv)
 }
 }
 
-out_bs = bdrv_new_open(out_filename, out_fmt);
+out_bs = bdrv_new_open(out_filename, out_fmt, 0);
 
 bs_i = 0;
 bs_offset = 0;
-- 
1.6.3.3

[Qemu-devel] [PATCH 0/4] block: more read-only changes, related to backing files

[Naphtali Sprei]

The read-only changes broke qemu-img create with read-only base image
These changes fix it.
It also make things a little safer, opening the backing file for read-only,
upgrading to read-write only for commit, and back to read-only when done.

Naphtali Sprei (4):
  Add open_flags to BlockDriverState Will be used later
  qemu-img: Fix qemu-img can't create qcow image based on read-only
image
  Block: readonly changes
  Open backing file read-only also for snapshot mode

 block.c |   57 +
 block_int.h |2 ++
 qemu-img.c  |   15 ++-
 3 files changed, 61 insertions(+), 13 deletions(-)

[Qemu-devel] [PATCH 1/4] Add open_flags to BlockDriverState Will be used later

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |1 +
 block_int.h |1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 1919d19..66564de 100644
--- a/block.c
+++ b/block.c
@@ -363,6 +363,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 bs-is_temporary = 0;
 bs-encrypted = 0;
 bs-valid_key = 0;
+bs-open_flags = flags;
 /* buffer_alignment defaulted to 512, drivers can change this value */
 bs-buffer_alignment = 512;
 
diff --git a/block_int.h b/block_int.h
index a0ebd90..9144d37 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 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 */
 int encrypted; /* if true, the media is encrypted */
-- 
1.6.3.3

[Qemu-devel] [PATCH 2/4] qemu-img: Fix qemu-img can't create qcow image based on read-only image

[Naphtali Sprei]

Open image file read-only where possible
Patch originally written by Sheng Yang sh...@linux.intel.com

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 qemu-img.c |   15 ++-
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index cbba4fc..b0ac9eb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -188,11 +188,13 @@ static int read_password(char *buf, int buf_size)
 #endif
 
 static BlockDriverState *bdrv_new_open(const char *filename,
-   const char *fmt)
+   const char *fmt,
+   int readonly)
 {
 BlockDriverState *bs;
 BlockDriver *drv;
 char password[256];
+int flags = BRDV_O_FLAGS;
 
 bs = bdrv_new();
 if (!bs)
@@ -204,7 +206,10 @@ static BlockDriverState *bdrv_new_open(const char 
*filename,
 } else {
 drv = NULL;
 }
-if (bdrv_open2(bs, filename, BRDV_O_FLAGS | BDRV_O_RDWR, drv)  0) {
+if (!readonly) {
+flags |= BDRV_O_RDWR;
+}
+if (bdrv_open2(bs, filename, flags, drv)  0) {
 error(Could not open '%s', filename);
 }
 if (bdrv_is_encrypted(bs)) {
@@ -343,7 +348,7 @@ static int img_create(int argc, char **argv)
 }
 }
 
-bs = bdrv_new_open(backing_file-value.s, fmt);
+bs = bdrv_new_open(backing_file-value.s, fmt, 1);
 bdrv_get_geometry(bs, size);
 size *= 512;
 bdrv_delete(bs);
@@ -627,7 +632,7 @@ static int img_convert(int argc, char **argv)
 
 total_sectors = 0;
 for (bs_i = 0; bs_i  bs_n; bs_i++) {
-bs[bs_i] = bdrv_new_open(argv[optind + bs_i], fmt);
+bs[bs_i] = bdrv_new_open(argv[optind + bs_i], fmt, 1);
 if (!bs[bs_i])
 error(Could not open '%s', argv[optind + bs_i]);
 bdrv_get_geometry(bs[bs_i], bs_sectors);
@@ -685,7 +690,7 @@ static int img_convert(int argc, char **argv)
 }
 }
 
-out_bs = bdrv_new_open(out_filename, out_fmt);
+out_bs = bdrv_new_open(out_filename, out_fmt, 0);
 
 bs_i = 0;
 bs_offset = 0;
-- 
1.6.3.3

[Qemu-devel] [PATCH 3/4] Block: readonly changes

[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 nsp...@redhat.com
---
 block.c |   64 +++---
 block_int.h |1 +
 2 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 66564de..527b146 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,34 @@ int bdrv_commit(BlockDriverState *bs)
 {
 BlockDriver *drv = bs-drv;
 int64_t i, total_sectors;
-int n, j;
+int n, j, ro;
 int ret = 0;
 unsigned char sector[512];
+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;
 
-if (!bs-backing_hd) {
-   return -ENOTSUP;
+if (ro) { /* re-open as RW */
+bs_rw = bdrv_new();
+ret = bdrv_open2(bs_rw, bs-backing_hd-filename,  
bs-backing_hd-open_flags | BDRV_O_RDWR, NULL);
+if (ret  0) {
+bdrv_delete(bs_rw);
+return -EACCES;
+}
+bdrv_close(bs-backing_hd);
+qemu_free(bs-backing_hd);
+bs-backing_hd = bs_rw;
 }
 
 total_sectors = bdrv_getlength(bs)  BDRV_SECTOR_BITS;
@@ -584,11 +614,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 +640,22 @@ int bdrv_commit(BlockDriverState *bs)
  */
 if (bs-backing_hd)
 bdrv_flush(bs-backing_hd);
+
+ro_cleanup:
+
+if (ro) { /* re-open as RO */
+bs_ro = bdrv_new();
+ret = bdrv_open2(bs_ro, bs-backing_hd-filename,  
bs-backing_hd-open_flags  ~BDRV_O_RDWR, NULL);
+if (ret  0) {
+bdrv_delete(bs_ro);
+return -EACCES;
+}
+bdrv_close(bs-backing_hd);
+qemu_free(bs-backing_hd);
+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

[Qemu-devel] [PATCH 4/4] Open backing file read-only also for snapshot mode

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |   10 +-
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 527b146..1db9961 100644
--- a/block.c
+++ b/block.c
@@ -483,19 +483,11 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 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);
-}
+open_flags = ~BDRV_O_RDWR;
 
 ret = bdrv_open2(bs-backing_hd, backing_filename, open_flags,
  back_drv);
 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;
 }
-- 
1.6.3.3

[Qemu-devel] Question on qcow2 image with base image

[Naphtali Sprei]

Hi,
when I use a qcow2 image based on a base image, what should happen when I 
invoke the commit command from the qemu monitor ?
Is it expected/intended to flush the data into the base image ?
IIUC, that is what happening in the released qemu (0.12).
I would expect it not to touch the base image.

  Naphtali

[Qemu-devel] Re: [PATCH v2] qemu-img: Fix qemu-img can't create qcow image based on read-only image

[Naphtali Sprei]

Sheng Yang wrote:
 Commit 03cbdac7 Disable fall-back to read-only when cannot open drive's
 file for read-write result in read-only image can't be used as backed
 image in qemu-img.
 
 Cc: Naphtali Sprei nsp...@redhat.com
 Signed-off-by: Sheng Yang sh...@linux.intel.com

Acked-by: Naphtali Sprei nsp...@redhat.com

Unfortunately, I see there are still problems with using the resulting images.
Investigating it, will send patches.

  Naphtali

[Qemu-devel] [PATCH] block: Enable fall-back to read-only for backing file

[Naphtali Sprei]

There's a problem when trying to use an image file based on a read-only image 
file.
Before this patch, qemu fails to open the base image and stop.
With this patch, qemu tries to open the backing file with same permissions as 
the top file,
but if it fails, qemu tries to open it with read-only permissions. If succeeded 
it goes on.

This fall-back works both for an image file based on a read-only file
and also for a read-only file opened with the snapshot attribute/mode (where 
the real file is the backing file
for the snapshot file).

Is it better to always open the backing file with read-only mode ? this will be 
more consistent/predictable ?
Or is it better not to fall-back to read-only ? Will a warning message help ?

 TIA,

   Naphtali
   

From 4a10750f5c91b1383118e4421f6b8d3ff3e79b2f Mon Sep 17 00:00:00 2001
From: Naphtali Sprei nsp...@redhat.com
Date: Sun, 31 Jan 2010 18:23:44 +0200
Subject: [PATCH] block: Enable fall-back to read-only for backing file

In order to use an image file backed by a read-only file, allow opening
the backing file with read-only permission.

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |5 +
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 1919d19..d1b0f3d 100644
--- a/block.c
+++ b/block.c
@@ -483,6 +483,11 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 back_drv = bdrv_find_format(bs-backing_format);
 ret = bdrv_open2(bs-backing_hd, backing_filename, open_flags,
  back_drv);
+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);
+}
 bs-backing_hd-read_only =  (open_flags  BDRV_O_RDWR) == 0;
 if (ret  0) {
 bdrv_close(bs);
-- 
1.6.3.3

[Qemu-devel] Re: [PATCH] Fix qemu-img can't create qcow image based on read-only image

[Naphtali Sprei]

Sheng Yang wrote:
 Commit 03cbdac7 Disable fall-back to read-only when cannot open drive's
 file for read-write result in read-only image can't be used as backed
 image in qemu-img.
 
 CC: Naphtali Sprei nsp...@redhat.com
 Signed-off-by: Sheng Yang sh...@linux.intel.com
 ---
 
 This issue blocked our QA's KVM nightly test. But in fact, I don't like this
 patch, feeling uncomfortable to change long existed interface... Any
 alternative? Add a readonly command line would change the default behavior(I
 don't think fall back to readonly looks like a bug); or even revert the
 commit? What's the story behind it?

The function I changed (bdrv_open) is shared between the qemu emulator and the 
qemu-img tool.
The read-only fall-back gave lots of troubles to the qemu emulator users.
The problem is that the drive was (silently) opened as read-only and the guest 
OS didn't know it's a read-only drive,
and tried to write to the drive and got all kind of mysterious failures.
My change tried to avoid this issue, with explicit message to the user, and 
stopping the emulator when this condition detected.

Your changes to the callers from qemu-img solves the problem nicely, except for 
the comment from Kevin.

  Naphtali

 
  qemu-img.c |   15 ++-
  1 files changed, 10 insertions(+), 5 deletions(-)
 
 diff --git a/qemu-img.c b/qemu-img.c
 index 3cea8ce..f8be5cb 100644
 --- a/qemu-img.c
 +++ b/qemu-img.c
 @@ -188,11 +188,13 @@ static int read_password(char *buf, int buf_size)
  #endif
  
  static BlockDriverState *bdrv_new_open(const char *filename,
 -   const char *fmt)
 +   const char *fmt,
 +   int readonly)
  {
  BlockDriverState *bs;
  BlockDriver *drv;
  char password[256];
 +int flags = BRDV_O_FLAGS;
  
  bs = bdrv_new();
  if (!bs)
 @@ -204,7 +206,10 @@ static BlockDriverState *bdrv_new_open(const char 
 *filename,
  } else {
  drv = NULL;
  }
 -if (bdrv_open2(bs, filename, BRDV_O_FLAGS | BDRV_O_RDWR, drv)  0) {
 +if (!readonly) {
 +flags |= BDRV_O_RDWR;
 +}
 +if (bdrv_open2(bs, filename, flags, drv)  0) {
  error(Could not open '%s', filename);
  }
  if (bdrv_is_encrypted(bs)) {
 @@ -343,7 +348,7 @@ static int img_create(int argc, char **argv)
  }
  }
  
 -bs = bdrv_new_open(backing_file-value.s, fmt);
 +bs = bdrv_new_open(backing_file-value.s, fmt, 1);
  bdrv_get_geometry(bs, size);
  size *= 512;
  bdrv_delete(bs);
 @@ -627,7 +632,7 @@ static int img_convert(int argc, char **argv)
  
  total_sectors = 0;
  for (bs_i = 0; bs_i  bs_n; bs_i++) {
 -bs[bs_i] = bdrv_new_open(argv[optind + bs_i], fmt);
 +bs[bs_i] = bdrv_new_open(argv[optind + bs_i], fmt, 0);
  if (!bs[bs_i])
  error(Could not open '%s', argv[optind + bs_i]);
  bdrv_get_geometry(bs[bs_i], bs_sectors);
 @@ -685,7 +690,7 @@ static int img_convert(int argc, char **argv)
  }
  }
  
 -out_bs = bdrv_new_open(out_filename, out_fmt);
 +out_bs = bdrv_new_open(out_filename, out_fmt, 0);
  
  bs_i = 0;
  bs_offset = 0;

[Qemu-devel] [PATCH 0/3] Even more read-write/read-only fixes and cleanup

[Naphtali Sprei]

A comment from Christoph Hellwig made me check (and fix) some stuff.
I'm afraid there are some more callers that needs to explicitly ask for 
read-write permissions.
I'd like to get comments about that.

P.S. The qcow2 bug (I introduced) in pre-allocate showed me there's a need to 
check return value.

Naphtali Sprei (3):
  No need anymore for bdrv_set_read_only
  Ask for read-write permissions when opening files where needed
  Read-only device changed to opens it's file for read-only.

 block.c   |7 ---
 block.h   |1 -
 block/bochs.c |6 ++
 block/parallels.c |6 ++
 block/qcow2.c |2 +-
 block/vvfat.c |2 +-
 qemu-img.c|4 ++--
 7 files changed, 8 insertions(+), 20 deletions(-)

[Qemu-devel] [PATCH 0/3] *** SUBJECT HERE ***

[Naphtali Sprei]

*** BLURB HERE ***

Naphtali Sprei (3):
  No need anymoe for bdrv_set_read_only
  Ask for read-write permissions when opening files
  Read-only device changed to opens it's file for read-only.

 block.c   |7 ---
 block.h   |1 -
 block/bochs.c |6 ++
 block/parallels.c |6 ++
 block/qcow2.c |2 +-
 block/vvfat.c |2 +-
 qemu-img.c|4 ++--
 7 files changed, 8 insertions(+), 20 deletions(-)

[Qemu-devel] [PATCH 0/3] *** SUBJECT HERE ***

[Naphtali Sprei]

*** BLURB HERE ***

Naphtali Sprei (3):
  No need anymore for bdrv_set_read_only
  Ask for read-write permissions when opening files where needed
  Read-only device changed to opens it's file for read-only.

 block.c   |7 ---
 block.h   |1 -
 block/bochs.c |6 ++
 block/parallels.c |6 ++
 block/qcow2.c |2 +-
 block/vvfat.c |2 +-
 qemu-img.c|4 ++--
 7 files changed, 8 insertions(+), 20 deletions(-)

[Qemu-devel] [PATCH 1/3] No need anymoe for bdrv_set_read_only

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |7 ---
 block.h |1 -
 2 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 73c26ec..8378c18 100644
--- a/block.c
+++ b/block.c
@@ -1009,13 +1009,6 @@ int bdrv_is_read_only(BlockDriverState *bs)
 return bs-read_only;
 }
 
-int bdrv_set_read_only(BlockDriverState *bs, int read_only)
-{
-int ret = bs-read_only;
-bs-read_only = read_only;
-return ret;
-}
-
 int bdrv_is_sg(BlockDriverState *bs)
 {
 return bs-sg;
diff --git a/block.h b/block.h
index fd4e8dd..1aec453 100644
--- a/block.h
+++ b/block.h
@@ -147,7 +147,6 @@ int bdrv_get_type_hint(BlockDriverState *bs);
 int bdrv_get_translation_hint(BlockDriverState *bs);
 int bdrv_is_removable(BlockDriverState *bs);
 int bdrv_is_read_only(BlockDriverState *bs);
-int bdrv_set_read_only(BlockDriverState *bs, int read_only);
 int bdrv_is_sg(BlockDriverState *bs);
 int bdrv_enable_write_cache(BlockDriverState *bs);
 int bdrv_is_inserted(BlockDriverState *bs);
-- 
1.6.3.3

[Qemu-devel] [PATCH 1/3] No need anymore for bdrv_set_read_only

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |7 ---
 block.h |1 -
 2 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 73c26ec..8378c18 100644
--- a/block.c
+++ b/block.c
@@ -1009,13 +1009,6 @@ int bdrv_is_read_only(BlockDriverState *bs)
 return bs-read_only;
 }
 
-int bdrv_set_read_only(BlockDriverState *bs, int read_only)
-{
-int ret = bs-read_only;
-bs-read_only = read_only;
-return ret;
-}
-
 int bdrv_is_sg(BlockDriverState *bs)
 {
 return bs-sg;
diff --git a/block.h b/block.h
index fd4e8dd..1aec453 100644
--- a/block.h
+++ b/block.h
@@ -147,7 +147,6 @@ int bdrv_get_type_hint(BlockDriverState *bs);
 int bdrv_get_translation_hint(BlockDriverState *bs);
 int bdrv_is_removable(BlockDriverState *bs);
 int bdrv_is_read_only(BlockDriverState *bs);
-int bdrv_set_read_only(BlockDriverState *bs, int read_only);
 int bdrv_is_sg(BlockDriverState *bs);
 int bdrv_enable_write_cache(BlockDriverState *bs);
 int bdrv_is_inserted(BlockDriverState *bs);
-- 
1.6.3.3

[Qemu-devel] [PATCH 2/3] Ask for read-write permissions when opening files

[Naphtali Sprei]

Found some places that seems needs this explicitly, now that
read-write is not the default.

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block/qcow2.c |2 +-
 block/vvfat.c |2 +-
 qemu-img.c|4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6622eba..91835f1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -961,7 +961,7 @@ static int qcow_create2(const char *filename, int64_t 
total_size,
 if (prealloc) {
 BlockDriverState *bs;
 bs = bdrv_new();
-bdrv_open(bs, filename, BDRV_O_CACHE_WB);
+bdrv_open(bs, filename, BDRV_O_CACHE_WB | BDRV_O_RDWR);
 preallocate(bs);
 bdrv_close(bs);
 }
diff --git a/block/vvfat.c b/block/vvfat.c
index 063f731..8e12f92 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2787,7 +2787,7 @@ static int enable_write_target(BDRVVVFATState *s)
 if (bdrv_create(bdrv_qcow, s-qcow_filename, options)  0)
return -1;
 s-qcow = bdrv_new();
-if (s-qcow == NULL || bdrv_open(s-qcow, s-qcow_filename, 0)  0)
+if (s-qcow == NULL || bdrv_open(s-qcow, s-qcow_filename, BDRV_O_RDWR)  
0)
return -1;
 
 #ifndef _WIN32
diff --git a/qemu-img.c b/qemu-img.c
index 3cea8ce..cbba4fc 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1116,7 +1116,7 @@ static int img_rebase(int argc, char **argv)
 if (!bs)
 error(Not enough memory);
 
-flags = BRDV_O_FLAGS | (unsafe ? BDRV_O_NO_BACKING : 0);
+flags = BRDV_O_FLAGS | BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0);
 if (bdrv_open2(bs, filename, flags, NULL)  0) {
 error(Could not open '%s', filename);
 }
@@ -1157,7 +1157,7 @@ static int img_rebase(int argc, char **argv)
 }
 
 bs_new_backing = bdrv_new(new_backing);
-if (bdrv_open2(bs_new_backing, out_baseimg, BRDV_O_FLAGS,
+if (bdrv_open2(bs_new_backing, out_baseimg, BRDV_O_FLAGS | BDRV_O_RDWR,
 new_backing_drv))
 {
 error(Could not open new backing file '%s', backing_name);
-- 
1.6.3.3

[Qemu-devel] [PATCH 2/3] Ask for read-write permissions when opening files where needed

[Naphtali Sprei]

Found some places that seems needs this explicitly, now that
read-write is not the default.

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block/qcow2.c |2 +-
 block/vvfat.c |2 +-
 qemu-img.c|4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6622eba..91835f1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -961,7 +961,7 @@ static int qcow_create2(const char *filename, int64_t 
total_size,
 if (prealloc) {
 BlockDriverState *bs;
 bs = bdrv_new();
-bdrv_open(bs, filename, BDRV_O_CACHE_WB);
+bdrv_open(bs, filename, BDRV_O_CACHE_WB | BDRV_O_RDWR);
 preallocate(bs);
 bdrv_close(bs);
 }
diff --git a/block/vvfat.c b/block/vvfat.c
index 063f731..8e12f92 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2787,7 +2787,7 @@ static int enable_write_target(BDRVVVFATState *s)
 if (bdrv_create(bdrv_qcow, s-qcow_filename, options)  0)
return -1;
 s-qcow = bdrv_new();
-if (s-qcow == NULL || bdrv_open(s-qcow, s-qcow_filename, 0)  0)
+if (s-qcow == NULL || bdrv_open(s-qcow, s-qcow_filename, BDRV_O_RDWR)  
0)
return -1;
 
 #ifndef _WIN32
diff --git a/qemu-img.c b/qemu-img.c
index 3cea8ce..cbba4fc 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1116,7 +1116,7 @@ static int img_rebase(int argc, char **argv)
 if (!bs)
 error(Not enough memory);
 
-flags = BRDV_O_FLAGS | (unsafe ? BDRV_O_NO_BACKING : 0);
+flags = BRDV_O_FLAGS | BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0);
 if (bdrv_open2(bs, filename, flags, NULL)  0) {
 error(Could not open '%s', filename);
 }
@@ -1157,7 +1157,7 @@ static int img_rebase(int argc, char **argv)
 }
 
 bs_new_backing = bdrv_new(new_backing);
-if (bdrv_open2(bs_new_backing, out_baseimg, BRDV_O_FLAGS,
+if (bdrv_open2(bs_new_backing, out_baseimg, BRDV_O_FLAGS | BDRV_O_RDWR,
 new_backing_drv))
 {
 error(Could not open new backing file '%s', backing_name);
-- 
1.6.3.3

[Qemu-devel] [PATCH 3/3] Read-only device changed to opens it's file for read-only.

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block/bochs.c |6 ++
 block/parallels.c |6 ++
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/block/bochs.c b/block/bochs.c
index 3489258..fb83594 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -116,11 +116,9 @@ static int bochs_open(BlockDriverState *bs, const char 
*filename, int flags)
 struct bochs_header bochs;
 struct bochs_header_v1 header_v1;
 
-fd = open(filename, O_RDWR | O_BINARY);
+fd = open(filename, O_RDONLY | O_BINARY);
 if (fd  0) {
-fd = open(filename, O_RDONLY | O_BINARY);
-if (fd  0)
-return -1;
+return -1;
 }
 
 bs-read_only = 1; // no write support yet
diff --git a/block/parallels.c b/block/parallels.c
index 63b6738..41b3a7c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -74,11 +74,9 @@ static int parallels_open(BlockDriverState *bs, const char 
*filename, int flags)
 int fd, i;
 struct parallels_header ph;
 
-fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE);
+fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
 if (fd  0) {
-fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
-if (fd  0)
-return -1;
+return -1;
 }
 
 bs-read_only = 1; // no write support yet
-- 
1.6.3.3

Re: [Qemu-devel] [PATCH v2 0/4] Modifications to the drives' readonly attribute

[Naphtali Sprei]

Christoph Hellwig wrote:
 Looking at the version of this that landed in git I don't think the
 read-only handling is entirely clean after this.

I fixed what I could, still I got some questions below.

 
  - we now normally set the read_only flag from bdrv_open2 when we do
not have the O_RDWR flag set
  - but the block drivers also mess with it:
   o raw-posix superflously sets it when BDRV_O_RDWR is not in the
 open flags

Not sure where exactly is the issue. Can you please point the line ?

   o bochs, cloop, dmg and parallels set it unconditionally given
 that they do not support writing at all.  But they do not
 bother to reject opens without BDRV_O_RDWR

I just changed bochs and parallels not to ask for read-write.
Should all of them test the flags for RDWR and returns failure ?

   o vvfat as usual is a complete mess setting and clearing it in
 various places

Fixed one occurance. More places ?

  - in addition to that bdrv_open2 also sets it after calling itself for
the backing hd which seems superflous

Is this a problem ? I thought it's safer to mark it read-only, in case a write 
operation requested somehow.

  - there also is a now unused bdrv_set_read_only helper to set it from
outside block.c

Done. Removed.

 
 

Thanks,

  Naphtali

[Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to exp

[Naphtali Sprei]

Michael S. Tsirkin wrote:
 On Sun, Jan 17, 2010 at 04:48:13PM +0200, Naphtali Sprei wrote:
 Instead of using the field 'readonly' of the BlockDriverState struct for 
 passing the request,
 pass the request in the flags parameter to the function.

 Signed-off-by: Naphtali Sprei nsp...@redhat.com
 
 Many changes seem to be about passing 0 to bdrv_open. This is not what
 the changelog says the patch does. Better split unrelated changes to a
 separate patch.

Thanks for the review.

Unfortunately, some of my comments went to the subject and are not part of the 
changelog.
So here's the (intended) changelog. This will clear-up some of your comments.

Clean-up a little bit the RW related bits of BDRV_O_FLAGS.
BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS).
Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request 
READ-WRITE.
Instead of using the field 'readonly' of the BlockDriverState struct for 
passing the request,
pass the request in the flags parameter to the function.



 
 One of the things you seem to do is get rid of BDRV_O_RDONLY.  Why is
 this an improvement? Symbolic name like BDRV_O_RDONLY seems better than
 0.

See Markus reply (thanks Markus).

 
 ---
  block.c   |   31 +--
  block.h   |2 --
  block/raw-posix.c |2 +-
  block/raw-win32.c |4 ++--
  block/vmdk.c  |9 +
  hw/xen_disk.c |2 +-
  monitor.c |2 +-
  qemu-img.c|   10 ++
  qemu-io.c |   14 ++
  qemu-nbd.c|2 +-
  vl.c  |8 
  11 files changed, 44 insertions(+), 42 deletions(-)

 diff --git a/block.c b/block.c
 index 115e591..8def3c4 100644
 --- a/block.c
 +++ b/block.c
 @@ -310,7 +310,7 @@ static BlockDriver *find_image_format(const char 
 *filename)
  if (drv  strcmp(drv-format_name, vvfat) == 0)
  return drv;
  
 -ret = bdrv_file_open(bs, filename, BDRV_O_RDONLY);
 +ret = bdrv_file_open(bs, filename, 0);
  if (ret  0)
  return NULL;
  ret = bdrv_pread(bs, 0, buf, sizeof(buf));
 @@ -356,7 +356,7 @@ int bdrv_open(BlockDriverState *bs, const char 
 *filename, int flags)
  int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
 BlockDriver *drv)
  {
 -int ret, open_flags, try_rw;
 +int ret, open_flags;
  char tmp_filename[PATH_MAX];
  char backing_filename[PATH_MAX];
  
 @@ -446,19 +446,23 @@ int bdrv_open2(BlockDriverState *bs, const char 
 *filename, int flags,
  
  /* Note: for compatibility, we open disk image files as RDWR, and
 RDONLY as fallback */
 -try_rw = !bs-read_only || bs-is_temporary;
 -if (!(flags  BDRV_O_FILE))
 -open_flags = (try_rw ? BDRV_O_RDWR : 0) |
 -(flags  (BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
 -else
 +bs-read_only = (flags  BDRV_O_RDWR) == 0;
 
 !(flags  BDRV_O_RDWR) is a standard idiom, and it's more readable IMO.
 
 +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 */
 +open_flags |= BDRV_O_RDWR;
 +}
 +} else {
  open_flags = flags  ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
 -if (use_bdrv_whitelist  !bdrv_is_whitelisted(drv))
 +}
 +if (use_bdrv_whitelist  !bdrv_is_whitelisted(drv)) {
  ret = -ENOTSUP;
 -else
 +} else {
  ret = drv-bdrv_open(bs, filename, open_flags);
 -if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE)) {
 -ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
 -bs-read_only = 1;
 +if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE)) {
 +ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
 +bs-read_only = 1;
 +}
  }
  if (ret  0) {
  qemu_free(bs-opaque);
 @@ -481,14 +485,13 @@ int bdrv_open2(BlockDriverState *bs, const char 
 *filename, int flags,
  /* if there is a backing file, use it */
  BlockDriver *back_drv = NULL;
  bs-backing_hd = bdrv_new();
 -/* pass on read_only property to the backing_hd */
 -bs-backing_hd-read_only = bs-read_only;
  path_combine(backing_filename, sizeof(backing_filename),
   filename, bs-backing_file);
  if (bs-backing_format[0] != '\0')
  back_drv = bdrv_find_format(bs-backing_format);
  ret = bdrv_open2(bs-backing_hd, backing_filename, open_flags,
   back_drv);
 +bs-backing_hd-read_only =  (open_flags  BDRV_O_RDWR) == 0;
 
 !(open_flags  BDRV_O_RDWR) is a standard idiom and it's more readable
 IMO.

Sorry, I prefer the more verbose style. The extra bytes on me. Seems more 
readable for me.

 Further, pls don't put two spaces after ==.

Sure, will correct.

 
  if (ret  0) {
  bdrv_close(bs);
  return

[Qemu-devel] Re: [PATCH v2 4/4] Disable fall-back to read-only when cannot open drive's file for read-write

[Naphtali Sprei]

Michael S. Tsirkin wrote:
 On Sun, Jan 17, 2010 at 04:48:15PM +0200, Naphtali Sprei wrote:
 Signed-off-by: Naphtali Sprei nsp...@redhat.com
 ---
  block.c |6 --
  1 files changed, 0 insertions(+), 6 deletions(-)

 diff --git a/block.c b/block.c
 index 8def3c4..f90e983 100644
 --- a/block.c
 +++ b/block.c
 @@ -444,8 +444,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;
  
 -/* Note: for compatibility, we open disk image files as RDWR, and
 -   RDONLY as fallback */
  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));
 @@ -459,10 +457,6 @@ int bdrv_open2(BlockDriverState *bs, const char 
 *filename, int flags,
  ret = -ENOTSUP;
  } else {
  ret = drv-bdrv_open(bs, filename, open_flags);
 -if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE)) {
 -ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
 -bs-read_only = 1;
 -}
 
 Maybe print an error message explaining the problem and
 suggesting the solution?

Printing done in (some of the) callers.
Should add suggestion. How is it done in QMP ?

 
  }
  if (ret  0) {
  qemu_free(bs-opaque);
 -- 
 1.6.3.3

[Qemu-devel] [PATCH v2 0/4] Modifications to the drives' readonly attribute

[Naphtali Sprei]

This is version 2, to replace previous set.
Addresses all Kevin comments.


Naphtali Sprei (4):
  Make CDROM a read-only drive
  Clean-up a little bit the RW related bits of BDRV_O_FLAGS.
BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for
bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request
READ-WRITE.
  Added drives' readonly option
  Disable fall-back to read-only when cannot open drive's file for
read-write

 block.c   |   29 +
 block.h   |2 --
 block/raw-posix.c |2 +-
 block/raw-win32.c |4 ++--
 block/vmdk.c  |9 +
 hw/xen_disk.c |2 +-
 monitor.c |2 +-
 qemu-img.c|   10 ++
 qemu-io.c |   14 ++
 qemu-nbd.c|2 +-
 qemu-options.hx   |2 +-
 vl.c  |   13 ++---
 12 files changed, 47 insertions(+), 44 deletions(-)

[Qemu-devel] [PATCH v2 1/4] Make CDROM a read-only drive

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 vl.c |7 +++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index 06cb40d..76ef8ca 100644
--- a/vl.c
+++ b/vl.c
@@ -2233,6 +2233,13 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
 }
 (void)bdrv_set_read_only(dinfo-bdrv, 1);
 }
+/* 
+ * cdrom is read-only. Set it now, after above interface checking
+ * since readonly attribute not explicitly required, so no error.
+ */
+if (media == MEDIA_CDROM) {
+(void)bdrv_set_read_only(dinfo-bdrv, 1);
+}
 
 if (bdrv_open2(dinfo-bdrv, file, bdrv_flags, drv)  0) {
 fprintf(stderr, qemu: could not open disk image %s: %s\n,
-- 
1.6.3.3

[Qemu-devel] [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explici

[Naphtali Sprei]

Instead of using the field 'readonly' of the BlockDriverState struct for 
passing the request,
pass the request in the flags parameter to the function.

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c   |   31 +--
 block.h   |2 --
 block/raw-posix.c |2 +-
 block/raw-win32.c |4 ++--
 block/vmdk.c  |9 +
 hw/xen_disk.c |2 +-
 monitor.c |2 +-
 qemu-img.c|   10 ++
 qemu-io.c |   14 ++
 qemu-nbd.c|2 +-
 vl.c  |8 
 11 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/block.c b/block.c
index 115e591..8def3c4 100644
--- a/block.c
+++ b/block.c
@@ -310,7 +310,7 @@ static BlockDriver *find_image_format(const char *filename)
 if (drv  strcmp(drv-format_name, vvfat) == 0)
 return drv;
 
-ret = bdrv_file_open(bs, filename, BDRV_O_RDONLY);
+ret = bdrv_file_open(bs, filename, 0);
 if (ret  0)
 return NULL;
 ret = bdrv_pread(bs, 0, buf, sizeof(buf));
@@ -356,7 +356,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, 
int flags)
 int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
BlockDriver *drv)
 {
-int ret, open_flags, try_rw;
+int ret, open_flags;
 char tmp_filename[PATH_MAX];
 char backing_filename[PATH_MAX];
 
@@ -446,19 +446,23 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 
 /* Note: for compatibility, we open disk image files as RDWR, and
RDONLY as fallback */
-try_rw = !bs-read_only || bs-is_temporary;
-if (!(flags  BDRV_O_FILE))
-open_flags = (try_rw ? BDRV_O_RDWR : 0) |
-(flags  (BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
-else
+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 */
+open_flags |= BDRV_O_RDWR;
+}
+} else {
 open_flags = flags  ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
-if (use_bdrv_whitelist  !bdrv_is_whitelisted(drv))
+}
+if (use_bdrv_whitelist  !bdrv_is_whitelisted(drv)) {
 ret = -ENOTSUP;
-else
+} else {
 ret = drv-bdrv_open(bs, filename, open_flags);
-if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE)) {
-ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
-bs-read_only = 1;
+if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE)) {
+ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
+bs-read_only = 1;
+}
 }
 if (ret  0) {
 qemu_free(bs-opaque);
@@ -481,14 +485,13 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 /* if there is a backing file, use it */
 BlockDriver *back_drv = NULL;
 bs-backing_hd = bdrv_new();
-/* pass on read_only property to the backing_hd */
-bs-backing_hd-read_only = bs-read_only;
 path_combine(backing_filename, sizeof(backing_filename),
  filename, bs-backing_file);
 if (bs-backing_format[0] != '\0')
 back_drv = bdrv_find_format(bs-backing_format);
 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) {
 bdrv_close(bs);
 return ret;
diff --git a/block.h b/block.h
index bee9ec5..fd4e8dd 100644
--- a/block.h
+++ b/block.h
@@ -27,9 +27,7 @@ typedef struct QEMUSnapshotInfo {
 uint64_t vm_clock_nsec; /* VM clock relative to boot */
 } QEMUSnapshotInfo;
 
-#define BDRV_O_RDONLY  0x
 #define BDRV_O_RDWR0x0002
-#define BDRV_O_ACCESS  0x0003
 #define BDRV_O_CREAT   0x0004 /* create an empty file */
 #define BDRV_O_SNAPSHOT0x0008 /* open the file read only and save writes 
in a snapshot */
 #define BDRV_O_FILE0x0010 /* open as a raw file (do not try to
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 5a6a22b..b427211 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -138,7 +138,7 @@ static int raw_open_common(BlockDriverState *bs, const char 
*filename,
 
 s-open_flags = open_flags | O_BINARY;
 s-open_flags = ~O_ACCMODE;
-if ((bdrv_flags  BDRV_O_ACCESS) == BDRV_O_RDWR) {
+if (bdrv_flags  BDRV_O_RDWR) {
 s-open_flags |= O_RDWR;
 } else {
 s-open_flags |= O_RDONLY;
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 72acad5..01a6d25 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -81,7 +81,7 @@ static int raw_open(BlockDriverState *bs, const char 
*filename, int flags)
 
 s-type = FTYPE_FILE;
 
-if ((flags  BDRV_O_ACCESS) == O_RDWR) {
+if (flags  BDRV_O_RDWR) {
 access_flags = GENERIC_READ

[Qemu-devel] [PATCH v2 3/4] Added drives' readonly option

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 qemu-options.hx |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index e2edd71..4617867 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -103,7 +103,7 @@ DEF(drive, HAS_ARG, QEMU_OPTION_drive,
 -drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n
[,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n
[,cache=writethrough|writeback|none][,format=f][,serial=s]\n
-   [,addr=A][,id=name][,aio=threads|native]\n
+   [,addr=A][,id=name][,aio=threads|native][,readonly=on|off]\n
 use 'file' as a drive image\n)
 DEF(set, HAS_ARG, QEMU_OPTION_set,
 -set group.id.arg=value\n
-- 
1.6.3.3

[Qemu-devel] [PATCH v2 4/4] Disable fall-back to read-only when cannot open drive's file for read-write

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |6 --
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 8def3c4..f90e983 100644
--- a/block.c
+++ b/block.c
@@ -444,8 +444,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;
 
-/* Note: for compatibility, we open disk image files as RDWR, and
-   RDONLY as fallback */
 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));
@@ -459,10 +457,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 ret = -ENOTSUP;
 } else {
 ret = drv-bdrv_open(bs, filename, open_flags);
-if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE)) {
-ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
-bs-read_only = 1;
-}
 }
 if (ret  0) {
 qemu_free(bs-opaque);
-- 
1.6.3.3

[Qemu-devel] [PATCH 0/3] Modifications to the drives' readonly attribute

[Naphtali Sprei]

Naphtali Sprei (3):
  Make CDROM a read-only drive
  Switch to bit-flags based read-only drive option implementation
  Disable fall-back to read-only when cannot open drive's file for
read-write

 block.c   |   29 +
 block.h   |4 ++--
 block/raw-posix.c |2 +-
 block/raw-win32.c |4 ++--
 block/vmdk.c  |9 +
 hw/xen_disk.c |2 +-
 monitor.c |2 +-
 qemu-img.c|   10 ++
 qemu-io.c |   14 ++
 qemu-nbd.c|2 +-
 qemu-options.hx   |2 +-
 vl.c  |   14 +++---
 12 files changed, 50 insertions(+), 44 deletions(-)

[Qemu-devel] [PATCH 1/3] Make CDROM a read-only drive

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 vl.c |4 
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index 06cb40d..4f19505 100644
--- a/vl.c
+++ b/vl.c
@@ -2234,6 +2234,10 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
 (void)bdrv_set_read_only(dinfo-bdrv, 1);
 }
 
+if (media == MEDIA_CDROM) {
+(void)bdrv_set_read_only(dinfo-bdrv, 1);
+}
+
 if (bdrv_open2(dinfo-bdrv, file, bdrv_flags, drv)  0) {
 fprintf(stderr, qemu: could not open disk image %s: %s\n,
 file, strerror(errno));
-- 
1.6.3.3

[Qemu-devel] [PATCH 3/3] Disable fall-back to read-only when cannot open drive's file for read-write

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c |4 
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index cbd72cc..0b4b9ad 100644
--- a/block.c
+++ b/block.c
@@ -457,10 +457,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 ret = -ENOTSUP;
 } else {
 ret = drv-bdrv_open(bs, filename, open_flags);
-if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE)) {
-ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
-bs-read_only = 1;
-}
 }
 if (ret  0) {
 qemu_free(bs-opaque);
-- 
1.6.3.3

Re: [Qemu-devel] [PATCH] Added 'access' option to -drive flag

[Naphtali Sprei]

Jamie Lokier wrote:
 Anthony Liguori wrote:
 On 12/24/2009 03:09 AM, Markus Armbruster wrote:
 Naphtali Spreinsp...@redhat.com  writes:

 Added 'access' option to -drive flag

 The new option is: access=[rw|ro|auto]
 rw: open the drive's file with Read and Write permission, don't continue 
 if failed
 ro: open the file only with Read permission
 auto: open the file with Read and Write permission, if failed, try only 
 Read permision

 For compatibility reasons, the default is 'auto'. Should be changed later 
 on.

 This option is to replace the 'readonly' options added lately.
 Can we take the readonly parameter away?  It's undocumented, for
 whatever that's worth...
 readonly made 0.12.   Semantics, readonly makes it to the disk emulation 
 whereas this effects how the file is opened.

I'm not sure I understand this semantic difference. The implementation of both 
versions (readonly and access) affects both
the disk emulation and the file access/open.
I did meant that 'access' to replace the 'readonly', and I do understand that I 
did it in bad timing.

 
 With readonly in 0.12, if you _don't specify readonly, and the file is
 opened readonly because it applies qemu's fallback behaviour - does
 *that* read-only property make it to the disk emulation?  Or do guests
 still see unexplained I/O errors in that case?

The implementation of both 'readonly' and 'access' pass the information to the 
Guest, through the device API.
Indeed, only for supporting devices.
 
 Btw, wasn't the access=[rw|ro|auto] option supposed to affect disk
 emulation too?
 
 -- Jamie

Re: [Qemu-devel] [PATCH] Added 'access' option to -drive flag

[Naphtali Sprei]

Simon Horman wrote:
 On Wed, Dec 23, 2009 at 02:12:23PM +0200, Naphtali Sprei wrote:
 Added 'access' option to -drive flag

 The new option is: access=[rw|ro|auto]
 rw: open the drive's file with Read and Write permission, don't continue if 
 failed
 ro: open the file only with Read permission
 auto: open the file with Read and Write permission, if failed, try only Read 
 permision

 For compatibility reasons, the default is 'auto'. Should be changed later on.
 
 What is the plan for changing the default later?

I don't have a plan, this is a product management issue.

 In particular, I'm curious to know why it will
 be able to be done later but not now.

There must be a way to give users advanced warning of a change coming, so they 
can get prepared for it.
Maybe also some deprecation process needed, don't know how is it done in QEMU.

 
 This option is to replace the 'readonly' options added lately.

 Instead of using the field 'readonly' of the BlockDriverState struct for 
 passing the request,
 pass the request in the flags parameter to the function.

 Signed-off-by: Naphtali Sprei nsp...@redhat.com
 ---
  block.c   |   27 +++
  block.h   |6 --
  block/raw-posix.c |2 +-
  block/raw-win32.c |4 ++--
  hw/xen_disk.c |3 ++-
  monitor.c |2 +-
  qemu-config.c |4 ++--
  qemu-img.c|   14 --
  qemu-nbd.c|2 +-
  qemu-options.hx   |2 +-
  vl.c  |   42 +-
  11 files changed, 70 insertions(+), 38 deletions(-)

 
 [snip]
 
 index e881e45..79b8c27 100644
 --- a/vl.c
 +++ b/vl.c
 @@ -2090,6 +2090,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
int *fatal_error)
  {
  const char *buf;
 +const char *access_buf = 0;
  const char *file = NULL;
  char devname[128];
  const char *serial;
 @@ -2104,12 +2105,12 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
  int index;
  int cache;
  int aio = 0;
 -int ro = 0;
  int bdrv_flags;
  int on_read_error, on_write_error;
  const char *devaddr;
  DriveInfo *dinfo;
  int snapshot = 0;
 +int read_write, ro_fallback;
  
  *fatal_error = 1;
  
 @@ -2137,7 +2138,6 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
  secs  = qemu_opt_get_number(opts, secs, 0);
  
  snapshot = qemu_opt_get_bool(opts, snapshot, 0);
 -ro = qemu_opt_get_bool(opts, readonly, 0);
  
  file = qemu_opt_get(opts, file);
  serial = qemu_opt_get(opts, serial);
 @@ -2420,6 +2420,31 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
  *fatal_error = 0;
  return NULL;
  }
 +
 +read_write = 1;
 +ro_fallback = 1;
 +access_buf = qemu_opt_get(opts, access);
 +if (access_buf) {
 +if (!strcmp(access_buf, ro)) {
 +read_write = 0;
 +ro_fallback = 0;
 +} else if (!strcmp(access_buf, rw)) {
 +read_write = 1;
 +ro_fallback = 0;
 +} else if (!strcmp(access_buf, auto)) { /* default, but keep it 
 explicit */
 +read_write = 1;
 +ro_fallback = 1;
 +} else {
 +fprintf(stderr, qemu: '%s' invalid 'access' option (valid 
 values: [rw|ro|auto] )\n, access_buf);
 +return NULL;
 +}
 +if (type != IF_SCSI  type != IF_VIRTIO  type != IF_FLOPPY 
 +( !strcmp(access_buf, ro) || !strcmp(access_buf, auto) )) {
 +fprintf(stderr, qemu: access=%s flag not supported for drive 
 of this interface\n, access_buf);
 +return NULL;
 +}
 +}
 +
 I wonder if it would be easier for the parsing
 code to use flags directly rather than abstracting
 things out to read_write, ro_fallback.

I felt this is more readable, even though it's a bit longer.

 
 e.g.
 
 (Sorry if I mucked up the logic)
 
 int access_flags = BDRV_O_RDWR | BDRV_O_RO_FALLBACK;
 access_buf = qemu_opt_get(opts, access);
 if (access_buf) {
 if (!strcmp(access_buf, ro))
   access_flags = BDRV_O_RDONLY;
   else if (!strcmp(access_buf, rw)) 
   access_flags = BDRV_O_RDWR;
   else if (!strcmp(access_buf, auto)) { /* default, but keep it 
 explicit */
   access_flags = BDRV_O_RDWR | BDRV_O_RO_FALLBACK;
   ...
 
   bdrv_flags |= access_flags;
 
 diff --git a/vl.c b/vl.c
  bdrv_flags = 0;
  if (snapshot) {
  bdrv_flags |= BDRV_O_SNAPSHOT;
 @@ -2436,16 +2461,15 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
  bdrv_flags = ~BDRV_O_NATIVE_AIO;
  }
  
 -if (ro == 1) {
 -if (type == IF_IDE) {
 -fprintf(stderr, qemu: readonly flag not supported for drive 
 with ide interface\n);
 -return NULL;
 -}
 -(void)bdrv_set_read_only(dinfo-bdrv, 1);
 +bdrv_flags |= read_write ? BDRV_O_RDWR : BDRV_O_RDONLY;
 +
 +if (ro_fallback

[Qemu-devel] [PATCH v2] Added 'access' option to -drive flag

[Naphtali Sprei]

The new option is: access=[rw|ro|auto]
rw: open the drive's file with Read and Write permission, don't continue if 
failed
ro: open the file only with Read permission
auto: open the file with Read and Write permission, if failed, try only Read 
permision

For compatibility reasons, the default is 'auto'. Should be changed later on.

This option replaces the 'readonly' options added lately.

Instead of using the field 'readonly' of the BlockDriverState struct for 
passing the request,
pass the request in the flags parameter to the function.

The BDRV_O_RDWR/RDONLY becomes just one bit, BDRV_O_RDONLY gone.
---
 block.c   |   27 +++
 block.h   |6 --
 block/raw-posix.c |2 +-
 block/raw-win32.c |4 ++--
 block/vmdk.c  |4 ++--
 hw/xen_disk.c |5 +++--
 monitor.c |2 +-
 qemu-config.c |4 ++--
 qemu-img.c|   14 --
 qemu-nbd.c|2 +-
 qemu-options.hx   |2 +-
 vl.c  |   48 +++-
 12 files changed, 79 insertions(+), 41 deletions(-)

diff --git a/block.c b/block.c
index 3f3496e..bdf140a 100644
--- a/block.c
+++ b/block.c
@@ -310,7 +310,7 @@ static BlockDriver *find_image_format(const char *filename)
 if (drv  strcmp(drv-format_name, vvfat) == 0)
 return drv;
 
-ret = bdrv_file_open(bs, filename, BDRV_O_RDONLY);
+ret = bdrv_file_open(bs, filename, ~BDRV_O_RDWR);
 if (ret  0)
 return NULL;
 ret = bdrv_pread(bs, 0, buf, sizeof(buf));
@@ -356,7 +356,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, 
int flags)
 int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
BlockDriver *drv)
 {
-int ret, open_flags, try_rw;
+int ret, open_flags;
 char tmp_filename[PATH_MAX];
 char backing_filename[PATH_MAX];
 
@@ -378,7 +378,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 
 /* if there is a backing file, use it */
 bs1 = bdrv_new();
-ret = bdrv_open2(bs1, filename, 0, drv);
+ret = bdrv_open2(bs1, filename, ~BDRV_O_RDWR, drv);
 if (ret  0) {
 bdrv_delete(bs1);
 return ret;
@@ -445,18 +445,22 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 bs-enable_write_cache = 1;
 
 /* Note: for compatibility, we open disk image files as RDWR, and
-   RDONLY as fallback */
-try_rw = !bs-read_only || bs-is_temporary;
-if (!(flags  BDRV_O_FILE))
-open_flags = (try_rw ? BDRV_O_RDWR : 0) |
-(flags  (BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
-else
+   RDONLY as fallback (if flag BDRV_O_RO_FALLBACK is on) */
+bs-read_only = BDRV_FLAGS_IS_RO(flags);
+if (!(flags  BDRV_O_FILE)) {
+open_flags = (flags  (BDRV_O_RDWR | 
BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
+if (bs-is_temporary) { /* snapshot */
+open_flags |= BDRV_O_RDWR;
+}
+} else {
 open_flags = flags  ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
+}
 if (use_bdrv_whitelist  !bdrv_is_whitelisted(drv))
 ret = -ENOTSUP;
 else
 ret = drv-bdrv_open(bs, filename, open_flags);
-if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE)) {
+if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE) 
+(flags  BDRV_O_RO_FALLBACK)) {
 ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
 bs-read_only = 1;
 }
@@ -481,14 +485,13 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 /* if there is a backing file, use it */
 BlockDriver *back_drv = NULL;
 bs-backing_hd = bdrv_new();
-/* pass on read_only property to the backing_hd */
-bs-backing_hd-read_only = bs-read_only;
 path_combine(backing_filename, sizeof(backing_filename),
  filename, bs-backing_file);
 if (bs-backing_format[0] != '\0')
 back_drv = bdrv_find_format(bs-backing_format);
 ret = bdrv_open2(bs-backing_hd, backing_filename, open_flags,
  back_drv);
+bs-backing_hd-read_only =  BDRV_FLAGS_IS_RO(open_flags);
 if (ret  0) {
 bdrv_close(bs);
 return ret;
diff --git a/block.h b/block.h
index fa51ddf..f2501be 100644
--- a/block.h
+++ b/block.h
@@ -28,8 +28,8 @@ typedef struct QEMUSnapshotInfo {
 } QEMUSnapshotInfo;
 
 #define BDRV_O_RDONLY  0x
-#define BDRV_O_RDWR0x0002
-#define BDRV_O_ACCESS  0x0003
+#define BDRV_O_RDWR0x0001
+#define BDRV_O_RO_FALLBACK 0x0002
 #define BDRV_O_CREAT   0x0004 /* create an empty file */
 #define BDRV_O_SNAPSHOT0x0008 /* open the file read only and save writes 
in a snapshot */
 #define BDRV_O_FILE0x0010 /* open as a raw file (do not try to
@@ -41,6 +41,8 @@ typedef struct QEMUSnapshotInfo {
 #define BDRV_O_NATIVE_AIO  0x0080 /* use native AIO 

[Qemu-devel] [PATCH] Added 'access' option to -drive flag

[Naphtali Sprei]

Added 'access' option to -drive flag

The new option is: access=[rw|ro|auto]
rw: open the drive's file with Read and Write permission, don't continue if 
failed
ro: open the file only with Read permission
auto: open the file with Read and Write permission, if failed, try only Read 
permision

For compatibility reasons, the default is 'auto'. Should be changed later on.

This option is to replace the 'readonly' options added lately.

Instead of using the field 'readonly' of the BlockDriverState struct for 
passing the request,
pass the request in the flags parameter to the function.

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c   |   27 +++
 block.h   |6 --
 block/raw-posix.c |2 +-
 block/raw-win32.c |4 ++--
 hw/xen_disk.c |3 ++-
 monitor.c |2 +-
 qemu-config.c |4 ++--
 qemu-img.c|   14 --
 qemu-nbd.c|2 +-
 qemu-options.hx   |2 +-
 vl.c  |   42 +-
 11 files changed, 70 insertions(+), 38 deletions(-)

diff --git a/block.c b/block.c
index 3f3496e..e8a48a4 100644
--- a/block.c
+++ b/block.c
@@ -356,7 +356,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, 
int flags)
 int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
BlockDriver *drv)
 {
-int ret, open_flags, try_rw;
+int ret, open_flags;
 char tmp_filename[PATH_MAX];
 char backing_filename[PATH_MAX];
 
@@ -378,7 +378,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 
 /* if there is a backing file, use it */
 bs1 = bdrv_new();
-ret = bdrv_open2(bs1, filename, 0, drv);
+ret = bdrv_open2(bs1, filename, BDRV_O_RDONLY, drv);
 if (ret  0) {
 bdrv_delete(bs1);
 return ret;
@@ -445,19 +445,23 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 bs-enable_write_cache = 1;
 
 /* Note: for compatibility, we open disk image files as RDWR, and
-   RDONLY as fallback */
-try_rw = !bs-read_only || bs-is_temporary;
-if (!(flags  BDRV_O_FILE))
-open_flags = (try_rw ? BDRV_O_RDWR : 0) |
-(flags  (BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
-else
+   RDONLY as fallback (if flag BDRV_O_RO_FALLBACK is on) */
+bs-read_only = BDRV_FLAGS_IS_RO(flags);
+if (!(flags  BDRV_O_FILE)) {
+open_flags = (flags  (BDRV_O_RDWR | 
BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
+if (bs-is_temporary) { /* snapshot */
+open_flags |= BDRV_O_RDWR;
+}
+} else {
 open_flags = flags  ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
+}
 if (use_bdrv_whitelist  !bdrv_is_whitelisted(drv))
 ret = -ENOTSUP;
 else
 ret = drv-bdrv_open(bs, filename, open_flags);
-if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE)) {
-ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
+if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE) 
+(flags  BDRV_O_RO_FALLBACK)) {
+ret = drv-bdrv_open(bs, filename, (open_flags  ~BDRV_O_RDWR) | 
BDRV_O_RDONLY);
 bs-read_only = 1;
 }
 if (ret  0) {
@@ -481,14 +485,13 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 /* if there is a backing file, use it */
 BlockDriver *back_drv = NULL;
 bs-backing_hd = bdrv_new();
-/* pass on read_only property to the backing_hd */
-bs-backing_hd-read_only = bs-read_only;
 path_combine(backing_filename, sizeof(backing_filename),
  filename, bs-backing_file);
 if (bs-backing_format[0] != '\0')
 back_drv = bdrv_find_format(bs-backing_format);
 ret = bdrv_open2(bs-backing_hd, backing_filename, open_flags,
  back_drv);
+bs-backing_hd-read_only =  BDRV_FLAGS_IS_RO(open_flags);
 if (ret  0) {
 bdrv_close(bs);
 return ret;
diff --git a/block.h b/block.h
index fa51ddf..f2501be 100644
--- a/block.h
+++ b/block.h
@@ -28,8 +28,8 @@ typedef struct QEMUSnapshotInfo {
 } QEMUSnapshotInfo;
 
 #define BDRV_O_RDONLY  0x
-#define BDRV_O_RDWR0x0002
-#define BDRV_O_ACCESS  0x0003
+#define BDRV_O_RDWR0x0001
+#define BDRV_O_RO_FALLBACK 0x0002
 #define BDRV_O_CREAT   0x0004 /* create an empty file */
 #define BDRV_O_SNAPSHOT0x0008 /* open the file read only and save writes 
in a snapshot */
 #define BDRV_O_FILE0x0010 /* open as a raw file (do not try to
@@ -41,6 +41,8 @@ typedef struct QEMUSnapshotInfo {
 #define BDRV_O_NATIVE_AIO  0x0080 /* use native AIO instead of the thread pool 
*/
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB)
+#define BDRV_O_DEFAULT_OPEN (BDRV_O_RDWR | BDRV_O_RO_FALLBACK)
+#define BDRV_FLAGS_IS_RO(flags) ((flags  BDRV_O_RDWR) == 0)
 
 #define BDRV_SECTOR_BITS   9
 #define

[Qemu-devel] [PATCH] Fix for cdrom un-eject

[Naphtali Sprei]

When guest un-eject a cdrom, re-insert the cdrom image (re-open the drive's 
file).

Also, related changes for the un-eject:
o  enter UNIT ATTENTION state only on change/insert media, not upon removal
o  minor change in packet command abort when in UNIT ATTENTION state (as per 
spec)
o  enter UNIT ATTENTION state upon reset (as per spec)
o  always print the file-name in info block command (if applicable)

Relevant Spec: Mt. Fuji Commands for Multimedia Devices Version 7: 
ftp://ftp.seagate.com/sff/INF-8090.PDF

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c   |   24 
 block.h   |1 +
 block_int.h   |1 +
 hw/ide/core.c |   35 +--
 monitor.c |1 +
 5 files changed, 48 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c
index 6fdabff..1801569 100644
--- a/block.c
+++ b/block.c
@@ -463,6 +463,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
 bs-read_only = 1;
 }
+bs-open_flags = flags;
 if (ret  0) {
 qemu_free(bs-opaque);
 bs-opaque = NULL;
@@ -1155,9 +1156,11 @@ void bdrv_info(Monitor *mon)
 if (bs-removable) {
 monitor_printf(mon,  locked=%d, bs-locked);
 }
-if (bs-drv) {
+if (bs-filename[0]) {
 monitor_printf(mon,  file=);
 monitor_print_filename(mon, bs-filename);
+}
+if (bs-drv) {
 if (bs-backing_file[0] != '\0') {
 monitor_printf(mon,  backing_file=);
 monitor_print_filename(mon, bs-backing_file);
@@ -1907,14 +1910,27 @@ int bdrv_eject(BlockDriverState *bs, int eject_flag)
 ret = drv-bdrv_eject(bs, eject_flag);
 }
 if (ret == -ENOTSUP) {
-if (eject_flag)
+if (eject_flag) {
 bdrv_close(bs);
-ret = 0;
+ret = 0;
+} else { /* re-insert media */
+if (bs-filename[0]) {
+ret = bdrv_open(bs, bs-filename, bs-open_flags);
+} else {
+ret = 0;
+}
+}
 }
-
+
 return ret;
 }
 
+
+void bdrv_forget_fname(BlockDriverState *bs)
+{
+bs-filename[0] = '\0';
+}
+
 int bdrv_is_locked(BlockDriverState *bs)
 {
 return bs-locked;
diff --git a/block.h b/block.h
index 2d4f066..8fc10d2 100644
--- a/block.h
+++ b/block.h
@@ -147,6 +147,7 @@ int bdrv_media_changed(BlockDriverState *bs);
 int bdrv_is_locked(BlockDriverState *bs);
 void bdrv_set_locked(BlockDriverState *bs, int locked);
 int bdrv_eject(BlockDriverState *bs, int eject_flag);
+void bdrv_forget_fname(BlockDriverState *bs);
 void bdrv_set_change_cb(BlockDriverState *bs,
 void (*change_cb)(void *opaque), void *opaque);
 void bdrv_get_format(BlockDriverState *bs, char *buf, int buf_size);
diff --git a/block_int.h b/block_int.h
index 7ebe926..1ecf7bf 100644
--- a/block_int.h
+++ b/block_int.h
@@ -137,6 +137,7 @@ struct BlockDriverState {
 void *opaque;
 
 char filename[1024];
+int  open_flags;
 char backing_file[1024]; /* if non zero, the image is a diff of
 this file image */
 char backing_format[16]; /* if non-zero and backing_file exists */
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 7b1ff8f..5631898 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -35,6 +35,20 @@
 
 #include hw/ide/internal.h
 
+
+static int is_pcommand_abort_on_check(uint8_t pcmd) {
+int ret = 1;
+
+if (pcmd == GPCMD_GET_CONFIGURATION ||
+pcmd == GPCMD_REQUEST_SENSE ||
+   pcmd == GPCMD_INQUIRY) {
+ret = 0;
+}
+return ret;
+}
+
+
+
 static int smart_attributes[][5] = {
 /* id,  flags, val, wrst, thrsh */
 { 0x01, 0x03, 0x64, 0x64, 0x06}, /* raw read */
@@ -1206,12 +1220,11 @@ static void ide_atapi_cmd(IDEState *s)
 }
 #endif
 /* If there's a UNIT_ATTENTION condition pending, only
-   REQUEST_SENSE and INQUIRY commands are allowed to complete. */
+   REQUEST_SENSE and INQUIRY commands (and some more) are allowed to 
complete. */
 if (s-sense_key == SENSE_UNIT_ATTENTION 
-   s-io_buffer[0] != GPCMD_REQUEST_SENSE 
-   s-io_buffer[0] != GPCMD_INQUIRY) {
-   ide_atapi_cmd_check_status(s);
-   return;
+is_pcommand_abort_on_check(s-io_buffer[0])) {
+ide_atapi_cmd_check_status(s);
+return;
 }
 switch(s-io_buffer[0]) {
 case GPCMD_TEST_UNIT_READY:
@@ -1676,9 +1689,11 @@ static void cdrom_change_cb(void *opaque)
 bdrv_get_geometry(s-bs, nb_sectors);
 s-nb_sectors = nb_sectors;
 
-s-sense_key = SENSE_UNIT_ATTENTION;
-s-asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
-s-cdrom_changed = 1;
+if (nb_sectors  0) { /* UNIT_ATTENTION only for insertion/change, not 
removal */
+s-sense_key = SENSE_UNIT_ATTENTION;
+s-asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
+s-cdrom_changed = 1

Re: [Qemu-devel] Stack corruption problem with SeaBIOS/gPXE under QEMU

[Naphtali Sprei]

Kevin O'Connor wrote:
 Hi,
 
 On Thu, Nov 12, 2009 at 01:20:58PM +0200, Naphtali Sprei wrote:
 I've found a problem with the usage of SeaBIOS/gPXE in Qemu.  The
 scenario is when failing to boot from network and falling back to
 booting from hard-disk (-boot nc).  The cause of the problem is that
 both SeaBIOS and gPXE (in it's installation phase) uses same stack
 area, 0x7c00.  The gPXE code corrupts the SeaBIOS stack, so when
 gPXE returns to SeaBIOS chaos occurs.

 Output: qemu: fatal: Trying to execute code outside RAM or ROM at 
 0xeb30
 
 Thanks for reporting this.
 
 We can move the SeaBIOS stack, but it's not clear to me where to move
 it to.  Bochs bios puts the top of the stack at 0x1, but this
 could potentially conflict with the OS load to 0x7c00.  So, in SeaBIOS
 the top of stack was moved to 0x7c00 to prevent this conflict.
 
 Maybe the gPXE developers know where the bios typically places its
 stack.
 
 However, I'm not sure why gPXE doesn't just use the stack it was
 given, or allocate the stack space it needs with PMM.
 
 A simple hack/patch (attached) solves this problem, but a proper
 patch expected from the SeaBIOS guys.

Sorry for the misleading addressee. I should have addressed the request to the 
gPXE project, and not SeaBIOS project.
Since the gPXE uses the services of SeaBIOS, that where the changes should be.
Thanks for CC'ing them. 


  Enjoy,

   Naphtali

 Patch against current SeaBIOS git
 
 The patch isn't against SeaBIOS.  Did you mean gPXE?

Sure, my mistake, against gPXE.

 
 -Kevin

[Qemu-devel] Stack corruption problem with SeaBIOS/gPXE under QEMU

[Naphtali Sprei]

Hi,
I've found a problem with the usage of SeaBIOS/gPXE in Qemu.
The scenario is when failing to boot from network and falling back to booting 
from hard-disk (-boot nc).
The cause of the problem is that both SeaBIOS and gPXE (in it's installation 
phase) uses same stack area, 0x7c00.
The gPXE code corrupts the SeaBIOS stack, so when gPXE returns to SeaBIOS chaos 
occurs.

Output: qemu: fatal: Trying to execute code outside RAM or ROM at 
0xeb30

A simple hack/patch (attached) solves this problem, but a proper patch expected 
from the SeaBIOS guys.

 Enjoy,

  Naphtali

Patch against current SeaBIOS git


Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 src/arch/i386/prefix/pxeprefix.S |2 +-
 src/arch/i386/prefix/romprefix.S |2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/arch/i386/prefix/pxeprefix.S b/src/arch/i386/prefix/pxeprefix.S
index b541e4b..11dd45d 100644
--- a/src/arch/i386/prefix/pxeprefix.S
+++ b/src/arch/i386/prefix/pxeprefix.S
@@ -47,7 +47,7 @@ FILE_LICENCE ( GPL2_OR_LATER )
/* Set up stack just below 0x7c00 */
xorw%ax, %ax
movw%ax, %ss
-   movl$0x7c00, %esp
+   movl$0x7a00, %esp
/* Clear direction flag, for the sake of sanity */
cld
/* Print welcome message */
diff --git a/src/arch/i386/prefix/romprefix.S b/src/arch/i386/prefix/romprefix.S
index cb474e8..93f3f17 100644
--- a/src/arch/i386/prefix/romprefix.S
+++ b/src/arch/i386/prefix/romprefix.S
@@ -587,7 +587,7 @@ exec:   /* Set %ds = %cs */
/* Obtain a reasonably-sized temporary stack */
xorw%ax, %ax
movw%ax, %ss
-   movw$0x7c00, %sp
+   movw$0x7a00, %sp
 
/* Install gPXE */
movlimage_source, %esi
-- 
1.6.3.3

[Qemu-devel] [PATCH] Pass the drive's readonly attribute to the guest OS

[Naphtali Sprei]

Hi,

I've seen my patch 
(http://repo.or.cz/w/qemu/aliguori-queue.git?a=commit;h=2286b94c7458cd6d72883990b53500194975c2ff)
in the staging tree, but the patch relies on a previous patch 
(http://lists.gnu.org/archive/html/qemu-devel/2009-10/msg01316.html)
I sent that I cannot find in the staging tree, nor committed.
So here is the missing patch again, against current head.

 Naphtali

Subject: [PATCH] Pass the drive's readonly attribute to the guest OS
 Implemented for virtio-blk and for scsi


Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 hw/scsi-disk.c  |3 ++-
 hw/virtio-blk.c |3 +++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 2a9268a..5da573d 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -633,7 +633,8 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t 
tag,
 memset(p, 0, 4);
 outbuf[1] = 0; /* Default media type.  */
 outbuf[3] = 0; /* Block descriptor length.  */
-if (bdrv_get_type_hint(s-dinfo-bdrv) == BDRV_TYPE_CDROM) {
+if (bdrv_get_type_hint(s-dinfo-bdrv) == BDRV_TYPE_CDROM ||
+bdrv_is_read_only(s-dinfo-bdrv)) {
 outbuf[2] = 0x80; /* Readonly.  */
 }
 p += 4;
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 2630b99..e6df9f2 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -444,6 +444,9 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
 #endif
 if (strcmp(s-serial_str, 0))
 features |= 1  VIRTIO_BLK_F_IDENTIFY;
+
+if (bdrv_is_read_only(s-bs))
+features |= 1  VIRTIO_BLK_F_RO;
 
 return features;
 }
-- 
1.6.3.3

[Qemu-devel] Usage of qemu_error withing shared object

[Naphtali Sprei]

Hi,
I've added a call to qemu_error in block.c file, and got a link error on the 
qemu-utilities: qemu-img, qemu-nbd and qemu-io.
Should there be a stub that implements a qemu_error to be linked with those 
apps ? Am I missing something ?

In my case, I couldn't let the caller report the error (and avoid calling 
qemu_error in block.c) since caller don't have enough information
for proper error message.


  Naphtali

[Qemu-devel] [PATCH] Added imlpementation for qemu_error for non-qemu executables

[Naphtali Sprei]

Now qemu_error can be called also from shared files, e.g. block.c.


Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 qemu-tool.c |9 +
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/qemu-tool.c b/qemu-tool.c
index ba24aa2..186c4f7 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -88,3 +88,12 @@ int64_t qemu_get_clock(QEMUClock *clock)
 qemu_gettimeofday(tv);
 return (tv.tv_sec * 10LL + (tv.tv_usec * 1000)) / 100;
 }
+
+void qemu_error(const char *fmt, ...)
+{
+va_list args;
+
+va_start(args, fmt);
+vfprintf(stderr, fmt, args);
+va_end(args);
+}
-- 
1.6.3.3

[Qemu-devel] [PATCH v2] Added readonly flag to -drive command

[Naphtali Sprei]

This is a slightly revised patch for adding readonly flag to the -drive command.
Even though this patch is stand-alone, it assumes a previous related patch 
(in Anthony staging tree), that passes
the readonly attribute of the drive to the guest OS, applied first.

This enables sharing same image between guests, with readonly access.
Implementaion mark the drive as read_only and changes the flags when actually 
opening the file.
The readonly attribute of a qcow also passed to it's base file.
For ide that cannot pass the readonly attribute to the guest OS, disallow the 
readonly flag.

Also, return error code from bdrv_truncate for readonly drive.

Signed-off-by: Naphtali Sprei nsp...@redhat.com
---
 block.c   |   19 +++
 block.h   |1 +
 qemu-config.c |3 +++
 vl.c  |   10 ++
 4 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 33f3d65..cffd95e 100644
--- a/block.c
+++ b/block.c
@@ -331,11 +331,10 @@ int bdrv_open(BlockDriverState *bs, const char *filename, 
int flags)
 int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
BlockDriver *drv)
 {
-int ret, open_flags;
+int ret, open_flags, try_rw;
 char tmp_filename[PATH_MAX];
 char backing_filename[PATH_MAX];
 
-bs-read_only = 0;
 bs-is_temporary = 0;
 bs-encrypted = 0;
 bs-valid_key = 0;
@@ -422,9 +421,10 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 
 /* Note: for compatibility, we open disk image files as RDWR, and
RDONLY as fallback */
+try_rw = !bs-read_only || bs-is_temporary;
 if (!(flags  BDRV_O_FILE))
-open_flags = BDRV_O_RDWR |
-   (flags  (BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
+open_flags = (try_rw ? BDRV_O_RDWR : 0) |
+(flags  (BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
 else
 open_flags = flags  ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
 ret = drv-bdrv_open(bs, filename, open_flags);
@@ -453,6 +453,8 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 /* if there is a backing file, use it */
 BlockDriver *back_drv = NULL;
 bs-backing_hd = bdrv_new();
+/* pass on read_only property to the backing_hd */
+bs-backing_hd-read_only = bs-read_only;
 path_combine(backing_filename, sizeof(backing_filename),
  filename, bs-backing_file);
 if (bs-backing_format[0] != '\0')
@@ -731,6 +733,8 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
 return -ENOMEDIUM;
 if (!drv-bdrv_truncate)
 return -ENOTSUP;
+if (bs-read_only)
+return -EACCES;
 return drv-bdrv_truncate(bs, offset);
 }
 
@@ -925,6 +929,13 @@ int bdrv_is_read_only(BlockDriverState *bs)
 return bs-read_only;
 }
 
+int bdrv_set_read_only(BlockDriverState *bs, int read_only)
+{
+int ret = bs-read_only;
+bs-read_only = read_only;
+return ret;
+}
+
 int bdrv_is_sg(BlockDriverState *bs)
 {
 return bs-sg;
diff --git a/block.h b/block.h
index a966afb..302010d 100644
--- a/block.h
+++ b/block.h
@@ -136,6 +136,7 @@ int bdrv_get_type_hint(BlockDriverState *bs);
 int bdrv_get_translation_hint(BlockDriverState *bs);
 int bdrv_is_removable(BlockDriverState *bs);
 int bdrv_is_read_only(BlockDriverState *bs);
+int bdrv_set_read_only(BlockDriverState *bs, int read_only);
 int bdrv_is_sg(BlockDriverState *bs);
 int bdrv_enable_write_cache(BlockDriverState *bs);
 int bdrv_is_inserted(BlockDriverState *bs);
diff --git a/qemu-config.c b/qemu-config.c
index cae92f7..4ac8fa0 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -71,6 +71,9 @@ QemuOptsList qemu_drive_opts = {
 .name = addr,
 .type = QEMU_OPT_STRING,
 .help = pci address (virtio only),
+},{
+.name = readonly,
+.type = QEMU_OPT_BOOL,
 },
 { /* end if list */ }
 },
diff --git a/vl.c b/vl.c
index eb2744e..97a940e 100644
--- a/vl.c
+++ b/vl.c
@@ -2007,6 +2007,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
 int index;
 int cache;
 int aio = 0;
+int ro = 0;
 int bdrv_flags, onerror;
 const char *devaddr;
 DriveInfo *dinfo;
@@ -2038,6 +2039,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
 secs  = qemu_opt_get_number(opts, secs, 0);
 
 snapshot = qemu_opt_get_bool(opts, snapshot, 0);
+ro = qemu_opt_get_bool(opts, readonly, 0);
 
 file = qemu_opt_get(opts, file);
 serial = qemu_opt_get(opts, serial);
@@ -2329,6 +2331,14 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
 bdrv_flags = ~BDRV_O_NATIVE_AIO;
 }
 
+if (ro == 1) {
+if (type == IF_IDE) {
+fprintf(stderr, qemu: readonly flag not supported for drive with 
ide interface\n);
+return NULL;
+}
+(void)bdrv_set_read_only(dinfo-bdrv, 1);
+}
+
 if (bdrv_open2(dinfo-bdrv, file, bdrv_flags, drv)  0