[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 <address@hidden>
> Signed-off-by: Sheng Yang <address@hidden>
> ---
> 
> 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;