[2/2] qemu-img: change opening method for the output in dd

Commit Message

Reda Sallahi Oct. 7, 2016, 3:36 p.m. UTC

The subcommand dd was creating an output image regardless of whether there
was one already created. With this patch we try to check first if the output
image exists and resize it if necessary.

Signed-off-by: Reda Sallahi <[e-mail]>
---
 qemu-img.c             | 124 ++++++++++++++++++++++++++++++-------------------
 tests/qemu-iotests/160 |   1 -
 2 files changed, 75 insertions(+), 50 deletions(-)

Comments

Max Reitz Oct. 10, 2016, 7:52 p.m. UTC | #1

On 07.10.2016 17:36, Reda Sallahi wrote:
> The subcommand dd was creating an output image regardless of whether there
> was one already created. With this patch we try to check first if the output
> image exists and resize it if necessary.
> 
> Signed-off-by: Reda Sallahi <[e-mail]>
> ---
>  qemu-img.c             | 124 ++++++++++++++++++++++++++++++-------------------
>  tests/qemu-iotests/160 |   1 -
>  2 files changed, 75 insertions(+), 50 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 6c088bd..9b590d4 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c

[...]

> @@ -4083,31 +4053,87 @@ static int img_dd(int argc, char **argv)

[...]

>      blk2 = img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR,
> -                    false, false, true);
> +                    false, false, false);
>  
>      if (!blk2) {

[...]

> +    } else {
> +        int64_t blk2sz = 0;
> +
> +        if (!(dd.conv & C_NOTRUNC)) {
> +            /* We make conv=notrunc mandatory for the moment to avoid
> +               accidental destruction of the output image. Needs to be
> +               changed when a better solution is found */
> +            error_report("conv=notrunc not specified");
> +            ret = -1;
> +            goto out;
> +        }
> +
> +        blk2sz = blk_getlength(blk2);
> +        if (blk2sz < 0) {
> +            error_report("Failed to get size for '%s'", in.filename);
> +            ret = -1;
> +            goto out;
> +        }
> +
> +        if (in.offset <= INT64_MAX / in.bsz && size >= in.offset * in.bsz) {
> +            if (blk2sz < out_size) {

Looks good to me overall, but wouldn't the second condition (blk2sz <
out_size) suffice alone? In my opinion, dropping the first "if" would
make the code easier to read, too.

Max

> +                blk_truncate(blk2, out_size);
> +            }
> +        }
>      }
>  
>      if (dd.flags & C_SKIP && (in.offset > INT64_MAX / in.bsz ||

Patch

diff --git a/qemu-img.c b/qemu-img.c
index 6c088bd..9b590d4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3938,7 +3938,7 @@  static int img_dd(int argc, char **argv)
     int c, i;
     const char *out_fmt = "raw";
     const char *fmt = NULL;
-    int64_t size = 0;
+    int64_t size = 0, out_size;
     int64_t block_count = 0, out_pos, in_pos;
     struct DdInfo dd = {
         .flags = 0,
@@ -4042,36 +4042,6 @@  static int img_dd(int argc, char **argv)
         goto out;
     }
 
-    drv = bdrv_find_format(out_fmt);
-    if (!drv) {
-        error_report("Unknown file format");
-        ret = -1;
-        goto out;
-    }
-    proto_drv = bdrv_find_protocol(out.filename, true, &local_err);
-
-    if (!proto_drv) {
-        error_report_err(local_err);
-        ret = -1;
-        goto out;
-    }
-    if (!drv->create_opts) {
-        error_report("Format driver '%s' does not support image creation",
-                     drv->format_name);
-        ret = -1;
-        goto out;
-    }
-    if (!proto_drv->create_opts) {
-        error_report("Protocol driver '%s' does not support image creation",
-                     proto_drv->format_name);
-        ret = -1;
-        goto out;
-    }
-    create_opts = qemu_opts_append(create_opts, drv->create_opts);
-    create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
-
-    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
-
     size = blk_getlength(blk1);
     if (size < 0) {
         error_report("Failed to get size for '%s'", in.filename);
@@ -4083,31 +4053,87 @@  static int img_dd(int argc, char **argv)
         dd.count * in.bsz < size) {
         size = dd.count * in.bsz;
     }
-
     /* Overflow means the specified offset is beyond input image's size */
-    if (dd.flags & C_SKIP && (in.offset > INT64_MAX / in.bsz ||
-                              size < in.bsz * in.offset)) {
-        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, 0, &error_abort);
+    if (in.offset > INT64_MAX / in.bsz || size < in.offset * in.bsz) {
+        out_size = 0;
     } else {
-        qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
-                            size - in.bsz * in.offset, &error_abort);
-    }
-
-    ret = bdrv_create(drv, out.filename, opts, &local_err);
-    if (ret < 0) {
-        error_reportf_err(local_err,
-                          "%s: error while creating output image: ",
-                          out.filename);
-        ret = -1;
-        goto out;
+        out_size = size - in.offset * in.bsz;
     }
 
     blk2 = img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR,
-                    false, false, true);
+                    false, false, false);
 
     if (!blk2) {
-        ret = -1;
-        goto out;
+        drv = bdrv_find_format(out_fmt);
+        if (!drv) {
+            error_report("Unknown file format");
+            ret = -1;
+            goto out;
+        }
+        proto_drv = bdrv_find_protocol(out.filename, true, &local_err);
+
+        if (!proto_drv) {
+            error_report_err(local_err);
+            ret = -1;
+            goto out;
+        }
+        if (!drv->create_opts) {
+            error_report("Format driver '%s' does not support image creation",
+                         drv->format_name);
+            ret = -1;
+            goto out;
+        }
+        if (!proto_drv->create_opts) {
+            error_report("Protocol driver '%s' does not support image creation",
+                         proto_drv->format_name);
+            ret = -1;
+            goto out;
+        }
+        create_opts = qemu_opts_append(create_opts, drv->create_opts);
+        create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
+
+        opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
+
+        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, out_size, &error_abort);
+
+        ret = bdrv_create(drv, out.filename, opts, &local_err);
+        if (ret < 0) {
+            error_reportf_err(local_err,
+                              "%s: error while creating output image: ",
+                              out.filename);
+            ret = -1;
+            goto out;
+        }
+        blk2 = img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR,
+                        false, false, true);
+        if (!blk2) {
+            ret = -1;
+            goto out;
+        }
+    } else {
+        int64_t blk2sz = 0;
+
+        if (!(dd.conv & C_NOTRUNC)) {
+            /* We make conv=notrunc mandatory for the moment to avoid
+               accidental destruction of the output image. Needs to be
+               changed when a better solution is found */
+            error_report("conv=notrunc not specified");
+            ret = -1;
+            goto out;
+        }
+
+        blk2sz = blk_getlength(blk2);
+        if (blk2sz < 0) {
+            error_report("Failed to get size for '%s'", in.filename);
+            ret = -1;
+            goto out;
+        }
+
+        if (in.offset <= INT64_MAX / in.bsz && size >= in.offset * in.bsz) {
+            if (blk2sz < out_size) {
+                blk_truncate(blk2, out_size);
+            }
+        }
     }
 
     if (dd.flags & C_SKIP && (in.offset > INT64_MAX / in.bsz ||
diff --git a/tests/qemu-iotests/160 b/tests/qemu-iotests/160
index f44834f..53b3c30 100755
--- a/tests/qemu-iotests/160
+++ b/tests/qemu-iotests/160
@@ -65,7 +65,6 @@  for skip in $TEST_SKIP_BLOCKS; do
     echo "== Compare the images with qemu-img compare =="
 
     $QEMU_IMG compare "$TEST_IMG.out.dd" "$TEST_IMG.out"
-    rm -f "$TEST_IMG.out.dd"
 done
 
 echo