Patchwork Make qemu-img convert properly consider backing file contents when used with -o backing_file

login
register
mail settings
Submitter Brad Campbell
Date April 29, 2011, 1:42 a.m.
Message ID <4DBA1782.7050600@fnarfbargle.com>
Download mbox | patch
Permalink /patch/93372/
State New
Headers show

Comments

Brad Campbell - April 29, 2011, 1:42 a.m.
G'day all,

This patch makes qemu-img properly consider the contents of the output 
backing file when performing a convert operation. All things considered 
it would also perform similar to rebase, where you could specify a 
completely different backing file and it would just de-dup.

I've poked this in as an attachment as apparently my last attempt at an 
in-line patch munged the formatting.

Comments, pokes or flames welcome.

Regards,
Brad
Kevin Wolf - April 29, 2011, 10:18 a.m.
Am 29.04.2011 03:42, schrieb Brad Campbell:
> G'day all,
> 
> This patch makes qemu-img properly consider the contents of the output 
> backing file when performing a convert operation. All things considered 
> it would also perform similar to rebase, where you could specify a 
> completely different backing file and it would just de-dup.
> 
> I've poked this in as an attachment as apparently my last attempt at an 
> in-line patch munged the formatting.
> 
> Comments, pokes or flames welcome.

Please include a commit comment and a Signed-off-by line. Instead of
sending a plain diff, you'll get a better result if you have a git
commit locally and use git format-patch to create the patch file.

Also please make sure to avoid trailing whitespace.

> > @@ -719,6 +720,16 @@ static int img_convert(int argc, char **argv)
>      out_baseimg_param = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
>      if (out_baseimg_param) {
>          out_baseimg = out_baseimg_param->value.s;
> +
> +        /* out_baseimg_parm != NULL even if there is no base img specified! */
> +        if (out_baseimg) {
> +            out_bf = bdrv_new_open(out_baseimg, NULL, BDRV_O_FLAGS);
> +            if (!out_bf) {
> +                error_report("Could not open backing file '%s'", out_baseimg);
> +                ret = -1;
> +                goto out;
> 

bdrv_new_open already prints an error message in case of failure, so we
shouldn't duplicate it here.

> @@ -889,8 +903,15 @@ static int img_convert(int argc, char **argv)
>                     are present in both the output's and input's base images (no
>                     need to copy them). */
>                  if (out_baseimg) {
> -                    if (!bdrv_is_allocated(bs[bs_i], sector_num - bs_offset,
> -                                           n, &n1)) {
> +                    if (bdrv_read(bs[bs_i], sector_num - bs_offset, buf, n) < 0) {
> +                        error_report("error while reading input file");
> +                        goto out;
> +                    }
> +                    if (bdrv_read(out_bf, sector_num - bs_offset, buf3, n) < 0) {
> +                        error_report("error while reading backing file");
> +                        goto out;
> +                    }

Did you test what happens with a backing file that is smaller than the
overlay? I suspect it will fail here after this change.

Also I think you need to set ret = -1 before jumping to out:

Kevin

Patch

diff --git a/qemu-img.c b/qemu-img.c
index d9c2c12..02455af 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -571,11 +571,12 @@  static int img_convert(int argc, char **argv)
     int progress = 0;
     const char *fmt, *out_fmt, *out_baseimg, *out_filename;
     BlockDriver *drv, *proto_drv;
-    BlockDriverState **bs = NULL, *out_bs = NULL;
+    BlockDriverState **bs = NULL, *out_bs = NULL, *out_bf = NULL;
     int64_t total_sectors, nb_sectors, sector_num, bs_offset;
     uint64_t bs_sectors;
     uint8_t * buf = NULL;
     const uint8_t *buf1;
+    uint8_t * buf3 = NULL;
     BlockDriverInfo bdi;
     QEMUOptionParameter *param = NULL, *create_options = NULL;
     QEMUOptionParameter *out_baseimg_param;
@@ -719,6 +720,16 @@  static int img_convert(int argc, char **argv)
     out_baseimg_param = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
     if (out_baseimg_param) {
         out_baseimg = out_baseimg_param->value.s;
+
+        /* out_baseimg_parm != NULL even if there is no base img specified! */
+        if (out_baseimg) {        
+            out_bf = bdrv_new_open(out_baseimg, NULL, BDRV_O_FLAGS);
+            if (!out_bf) {
+                error_report("Could not open backing file '%s'", out_baseimg);
+                ret = -1;
+                goto out;
+            }
+        }
     }
 
     /* Check if compression is supported */
@@ -767,6 +778,9 @@  static int img_convert(int argc, char **argv)
     bs_offset = 0;
     bdrv_get_geometry(bs[0], &bs_sectors);
     buf = qemu_malloc(IO_BUF_SIZE);
+    if (out_baseimg) {
+        buf3 = qemu_malloc(IO_BUF_SIZE);
+    }
 
     if (compress) {
         ret = bdrv_get_info(out_bs, &bdi);
@@ -889,8 +903,15 @@  static int img_convert(int argc, char **argv)
                    are present in both the output's and input's base images (no
                    need to copy them). */
                 if (out_baseimg) {
-                    if (!bdrv_is_allocated(bs[bs_i], sector_num - bs_offset,
-                                           n, &n1)) {
+                    if (bdrv_read(bs[bs_i], sector_num - bs_offset, buf, n) < 0) {
+                        error_report("error while reading input file");
+                        goto out;
+                    }
+                    if (bdrv_read(out_bf, sector_num - bs_offset, buf3, n) < 0) {
+                        error_report("error while reading backing file");
+                        goto out;
+                    }
+                    if (!compare_sectors(buf, buf3, n, &n1)) {
                         sector_num += n1;
                         continue;
                     }
@@ -939,9 +960,13 @@  out:
     free_option_parameters(create_options);
     free_option_parameters(param);
     qemu_free(buf);
+    qemu_free(buf3);
     if (out_bs) {
         bdrv_delete(out_bs);
     }
+    if (out_bf) {
+        bdrv_delete(out_bf);
+    }
     if (bs) {
         for (bs_i = 0; bs_i < bs_n; bs_i++) {
             if (bs[bs_i]) {