Fix qemu-img can't create qcow image based on read-only image

PATCH Superseded Sheng Yang 3 participants 4 messages Jan 28, 2010 – Jan 29, 2010 KVM

Share & tools

Sheng Yang

author

2010-01-28 05:23 UTC #0 lore raw

PATCH 1/1 Fix qemu-img can't create qcow image based on read-only image

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 <[email protected]>

Signed-off-by Sheng Yang <[email protected]>

---

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?

 qemu-img.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

qemu-img.c +10 -5

index 3cea8ce..f8be5cb 100644
@@ -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;

Signature · 1 line

1.5.4.5

Kevin Wolf

2010-01-28 09:18 UTC #1 lore raw

Am 28.01.2010 06:22, schrieb Sheng Yang:

Quoted 58 lines

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 <[email protected]>

Signed-off-by Sheng Yang <[email protected]>

---

 qemu-img.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

qemu-img.c +8 -3

index 3cea8ce..f8be5cb 100644
@@ -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);

qemu-img.c static int img_convert(int argc, char **argv) · line 635

Quoted 5 lines

qemu-img.c +1 -1

@@ -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);

Shouldn't it be read-only here, too?

Kevin

Sheng Yang

author

2010-01-29 02:14 UTC #2 lore raw

Quoted 60 lines · On Thursday 28 January 2010 17:17:22 Kevin Wolf wrote:

Am 28.01.2010 06:22, schrieb Sheng Yang:

Quoted 59 lines

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 <[email protected]>

Signed-off-by Sheng Yang <[email protected]>

---

 qemu-img.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

qemu-img.c +8 -3

index 3cea8ce..f8be5cb 100644
@@ -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)

Quoted 6 lines

qemu-img.c static int img_convert(int argc, char **argv) · line 635
Quoted 4 lines
qemu-img.c +1 -1
     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);
Shouldn't it be read-only here, too?

Yes.

Seems you guys are OK with this solution(and I think it's reasonable to remove the fall back). I would update the patch, hopefully it can be applied soon and picked up by Marcelo to resolve our block issue...

Signature · 2 lines

regards
Yang, Sheng

Naphtali Sprei

2010-01-28 09:41 UTC #3 lore raw

In reply to Sheng Yang

Quoted 6 lines · Sheng Yang

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 <[email protected]>
Signed-off-by Sheng Yang <[email protected]>

Quoted 7 lines

---
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

Quoted 62 lines

 qemu-img.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

qemu-img.c +10 -5

index 3cea8ce..f8be5cb 100644
@@ -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;