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

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

[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 <nsprei@redhat.com>
Signed-off-by: Sheng Yang <sheng@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?

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

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

[Kevin Wolf]

Am 28.01.2010 06:22, schrieb 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 <nsprei@redhat.com>
> Signed-off-by: Sheng Yang <sheng@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?
> 
>  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);

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

Kevin

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

[Sheng Yang]

On Thursday 28 January 2010 17:17:22 Kevin Wolf wrote:
> Am 28.01.2010 06:22, schrieb 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 <nsprei@redhat.com>
> > Signed-off-by: Sheng Yang <sheng@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?
> >
> >  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);
> 
> 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...

-- 
regards
Yang, Sheng

[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 <nsprei@redhat.com>
> Signed-off-by: Sheng Yang <sheng@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;