diff mbox

[v2,3/7] qemu-img: add more conv= conversions to dd

Message ID 20160826091643.15841-4-fullmanet@gmail.com
State New
Headers show

Commit Message

Reda Sallahi Aug. 26, 2016, 9:16 a.m. UTC
This patch adds excl, nocreat, noerror, sync, fsync, fdatasync and sparse to
the conversion list. They have the same meaning as the ones on GNU dd(1).

Two tests were added to test the conv= option.

Signed-off-by: Reda Sallahi <fullmanet@gmail.com>
---
 qemu-img-cmds.hx           |   4 +-
 qemu-img.c                 | 129 +++++++++++++++++++++++++++++++++++----------
 qemu-img.texi              |  26 +++++++--
 tests/qemu-iotests/165     | 109 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/165.out |  33 ++++++++++++
 tests/qemu-iotests/166     |  73 +++++++++++++++++++++++++
 tests/qemu-iotests/166.out |  19 +++++++
 tests/qemu-iotests/group   |   2 +
 8 files changed, 363 insertions(+), 32 deletions(-)
 create mode 100755 tests/qemu-iotests/165
 create mode 100644 tests/qemu-iotests/165.out
 create mode 100755 tests/qemu-iotests/166
 create mode 100644 tests/qemu-iotests/166.out

Comments

Stefan Hajnoczi Sept. 13, 2016, 4:06 p.m. UTC | #1
On Fri, Aug 26, 2016 at 11:16:39AM +0200, Reda Sallahi wrote:
> @@ -3968,7 +3963,11 @@ static int img_dd_flag(const char *arg, struct DdIo *io,
>          int j;
>          for (j = 0; flags[j].name != NULL; j++) {
>              if (!strcmp(tok, flags[j].name)) {
> -                io->flags |= flags[j].value;
> +                if (dd) {
> +                    dd->conv |= flags[j].value;
> +                } else {
> +                    io->flags |= flags[j].value;
> +                }
>                  break;
>              }
>          }

Does this function need to know about DdIo and DdInfo?  How about:

static int img_dd_flag(const char *arg, unsigned int *flag_bits,
                       const struct DdSymbols *flags, const char *err_str)

Then function can be called with &dd->flags or &io->flags.

If you do this then please squash the change into earlier patches in
this series using git-rebase(1).  That way the function is created with
its final prototype in the earliest patch that needs it and there is no
code churn.

> @@ -4219,6 +4260,7 @@ static int img_dd(int argc, char **argv)
>              ret = -1;
>              goto out;
>          }
> +        local_err = NULL;

Why set local_err to NULL here and not where it's declared?  If it could
possibly be non-NULL then error_free() must be used to avoid a memory
leak.

> +        if (dd.conv & C_SPARSE) {
> +            if (buffer_is_zero(in.buf, bsz)) {
> +                sparse_count++;
> +                continue;
> +            }
> +            if (sparse_count > 0) {
> +                out_pos += sparse_count * in.bsz;
> +                sparse_count = 0;
> +            }
> +        }

Is sparse_count needed at all?

if ((dd.conv & C_SPARSE) && buffer_is_zero(in.buf, bsz)) {
    out_pos += in.bsz;
    continue;
}
diff mbox

Patch

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 25eaf71..933ce3c 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -46,9 +46,9 @@  STEXI
 ETEXI
 
 DEF("dd", img_dd,
-    "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] [seek=blocks] [conv=notrunc] [iflag=flags] [oflag=flags] if=input of=output")
+    "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] [seek=blocks] [conv=convs] [iflag=flags] [oflag=flags] if=input of=output")
 STEXI
-    @item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] [seek=@var{blocks}] [conv=notrunc] [iflag=@var{flags}] [oflag=@var{flags}] if=@var{input} of=@var{output}
+    @item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] [seek=@var{blocks}] [conv=@var{convs}] [iflag=@var{flags}] [oflag=@var{flags}] if=@var{input} of=@var{output}
 ETEXI
 
 DEF("info", img_info,
diff --git a/qemu-img.c b/qemu-img.c
index 7b2c525..a4d0556 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -176,9 +176,19 @@  static void QEMU_NORETURN help(void)
            "  'of=FILE' write to FILE\n"
            "  'skip=N' skip N bs-sized blocks at the start of input\n"
            "  'seek=N' seek N bs-sized blocks at the start of output\n"
-           "  'conv=notrunc' do not truncate the output file\n"
+           "  'conv=CONVS' do not truncate the output file\n"
            "  'iflags=FLAGS' read using the comma-separated flags list\n"
            "  'oflags=FLAGS' read using the comma-separated flags list\n\n"
+           "List of CONVS for dd:\n"
+           "  'notrunc'   do not truncate the output file\n"
+           "  'noerror'   continue in the event of read errors\n"
+           "  'excl'      fail if output already exists\n"
+           "  'nocreat'   do not create the output file\n"
+           "  'fsync'     physically write output file data before finishing\n"
+           "  'fdatasync' physically write output file data before finishing\n"
+           "  'sync'      pad every input block with NULs\n"
+           "  'sparse'    seek rather than write the output for NUL input"
+           " blocks\n\n"
            "List of FLAGS for dd:\n"
            "  'direct'      use direct I/O for data\n"
            "  'dsync'       use synchronized I/O for data\n"
@@ -3932,21 +3942,6 @@  static int img_dd_seek(const char *arg,
     return 0;
 }
 
-#define C_NOTRUNC 01
-
-static int img_dd_conv(const char *arg,
-                       struct DdIo *in, struct DdIo *out,
-                       struct DdInfo *dd)
-{
-    if (!strcmp(arg, "notrunc")) {
-        dd->conv |= C_NOTRUNC;
-        return 0;
-    } else {
-        error_report("invalid conversion: '%s'", arg);
-        return 1;
-    }
-}
-
 #define C_DIRECT      01
 #define C_IOFLAG_SYNC 02
 #define C_DSYNC       04
@@ -3954,7 +3949,7 @@  static int img_dd_conv(const char *arg,
 #define C_SKIP_BYTES  020
 #define C_SEEK_BYTES  040
 
-static int img_dd_flag(const char *arg, struct DdIo *io,
+static int img_dd_flag(const char *arg, struct DdIo *io, struct DdInfo *dd,
                        const struct DdSymbols *flags, const char *err_str)
 {
     int ret = 0;
@@ -3968,7 +3963,11 @@  static int img_dd_flag(const char *arg, struct DdIo *io,
         int j;
         for (j = 0; flags[j].name != NULL; j++) {
             if (!strcmp(tok, flags[j].name)) {
-                io->flags |= flags[j].value;
+                if (dd) {
+                    dd->conv |= flags[j].value;
+                } else {
+                    io->flags |= flags[j].value;
+                }
                 break;
             }
         }
@@ -3996,7 +3995,7 @@  static int img_dd_iflag(const char *arg,
         { NULL, 0}
     };
 
-    return img_dd_flag(arg, in, flags, "invalid input flag");
+    return img_dd_flag(arg, in, NULL, flags, "invalid input flag");
 }
 
 static int img_dd_oflag(const char *arg,
@@ -4011,9 +4010,46 @@  static int img_dd_oflag(const char *arg,
         { NULL, 0 }
     };
 
-    return img_dd_flag(arg, out, flags, "invalid output flag");
+    return img_dd_flag(arg, out, NULL, flags, "invalid output flag");
+}
+
+#define C_NOTRUNC   01
+#define C_SYNC      02
+#define C_NOERROR   04
+#define C_FDATASYNC 010
+#define C_FSYNC     020
+#define C_EXCL      040
+#define C_NOCREAT   0100
+#define C_SPARSE    0200
+
+static int img_dd_conv(const char *arg,
+                       struct DdIo *in, struct DdIo *out,
+                       struct DdInfo *dd)
+{
+    int ret;
+    const struct DdSymbols conv[] = {
+        { "notrunc", C_NOTRUNC },
+        { "sync", C_SYNC },
+        { "noerror", C_NOERROR },
+        { "fdatasync", C_FDATASYNC },
+        { "fsync", C_FSYNC },
+        { "excl", C_EXCL },
+        { "nocreat", C_NOCREAT },
+        { "sparse", C_SPARSE },
+        { NULL, 0 }
+    };
+
+    ret = img_dd_flag(arg, NULL, dd, conv, "invalid conversion");
+
+    if (ret == 0 && dd->conv & C_EXCL && dd->conv & C_NOCREAT) {
+        error_report("cannot combine excl and nocreat");
+        ret = 1;
+    }
+
+    return ret;
 }
 
+
 static int img_dd(int argc, char **argv)
 {
     int ret = 0;
@@ -4030,10 +4066,10 @@  static int img_dd(int argc, char **argv)
     const char *fmt = NULL;
     const char *out_filename;
     int64_t size = 0, out_size = 0;
-    int64_t block_count = 0, out_pos, in_pos;
+    int64_t block_count = 0, out_pos, in_pos, sparse_count = 0;
     bool writethrough = false;
     int flags = 0;
-    int ibsz = 0, obsz = 0;
+    int ibsz = 0, obsz = 0, bsz;
     struct DdInfo dd = {
         .flags = 0,
         .count = 0,
@@ -4212,6 +4248,11 @@  static int img_dd(int argc, char **argv)
     ret = access(out_filename, F_OK); /* Check if file exists */
 
     if (ret == -1) {
+        if (dd.conv & C_NOCREAT) {
+            error_report("Failed to open '%s': %s",
+                         out_filename, strerror(errno));
+            goto out;
+        }
         ret = 0; /* Reset */
         drv = bdrv_find_format(out_fmt);
         if (!drv) {
@@ -4219,6 +4260,7 @@  static int img_dd(int argc, char **argv)
             ret = -1;
             goto out;
         }
+        local_err = NULL;
         proto_drv = bdrv_find_protocol(out.filename, true, &local_err);
 
         if (!proto_drv) {
@@ -4264,6 +4306,12 @@  static int img_dd(int argc, char **argv)
     } else {
         int64_t blk2sz = 0;
 
+        if (dd.conv & C_EXCL) {
+            error_report("failed to open '%s': File exists", out.filename);
+            ret = -1;
+            goto out;
+        }
+
         blk2 = img_open(image_opts, out.filename, out_fmt, flags,
                         writethrough, false);
         if (!blk2) {
@@ -4311,20 +4359,43 @@  static int img_dd(int argc, char **argv)
 
     for (out_pos = out.offset * obsz; in_pos < size; block_count++) {
         int in_ret, out_ret;
+        bsz = in.bsz;
 
         if (in_pos + in.bsz > size) {
-            in_ret = blk_pread(blk1, in_pos, in.buf, size - in_pos);
-        } else {
-            in_ret = blk_pread(blk1, in_pos, in.buf, in.bsz);
+            bsz = size - in_pos;
         }
+
+        if (dd.conv & C_SYNC) {
+            memset(in.buf, 0, in.bsz);
+        }
+        in_ret = blk_pread(blk1, in_pos, in.buf, bsz);
+
         if (in_ret < 0) {
             error_report("error while reading from input image file: %s",
                          strerror(-in_ret));
-            ret = -1;
-            goto out;
+            if (!(dd.conv & C_NOERROR)) {
+                ret = -1;
+                goto out;
+            }
+            if (!(dd.conv & C_SYNC)) {
+                in_pos += bsz;
+                continue;
+            }
+            in_ret = bsz;
         }
         in_pos += in_ret;
 
+        if (dd.conv & C_SPARSE) {
+            if (buffer_is_zero(in.buf, bsz)) {
+                sparse_count++;
+                continue;
+            }
+            if (sparse_count > 0) {
+                out_pos += sparse_count * in.bsz;
+                sparse_count = 0;
+            }
+        }
+
         out_ret = blk_pwrite(blk2, out_pos, in.buf, in_ret, 0);
 
         if (out_ret < 0) {
@@ -4336,6 +4407,10 @@  static int img_dd(int argc, char **argv)
         out_pos += out_ret;
     }
 
+    if (dd.conv & C_FDATASYNC || dd.conv & C_FSYNC) {
+        blk_flush(blk2);
+    }
+
 out:
     g_free(arg);
     qemu_opts_del(opts);
diff --git a/qemu-img.texi b/qemu-img.texi
index c663cf4..85c3cd3 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -159,8 +159,28 @@  specified, 'blocks' is interpreted as a byte count instead of a block count.
 @item seek=@var{blocks}
 sets the number of output blocks to skip. In case 'oflags=seek_bytes' is
 specified, 'blocks' is interpreted as a byte count instead of a block count.
-@item conv=notrunc
-makes dd not truncate output file to zero
+@item conv=@var{convs}
+Converts the input according to the conversions specified. The conversion list
+is separated using commas.
+
+The conversion list:
+@item notrunc
+Make dd not truncate output file to zero.
+@item sync
+Fill every input block size with trailing zero bytes.
+@item noerror
+Continue after read failures.
+@item fdatasync
+Synchronize output data just before finishing. This forces a physical write of output data.
+@item fsync
+Synchronize output data just before finishing. This forces a physical write of output data.
+@item excl
+Err if the output file already exists.
+@item nocreat
+Do not create the output file; the output file must already exist.
+@item sparse
+Seek rather than write NUL output blocks.
+
 @item iflag=@var{flags}
 defines the flags used to read the input file. The flag list is seprated using
 commas.
@@ -355,7 +375,7 @@  skipped. This is useful for formats such as @code{rbd} if the target
 volume has already been created with site specific options that cannot
 be supplied through qemu-img.
 
-@item dd [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] [seek=@var{blocks}] [conv=notrunc] [iflag=@var{flags}] [oflag=@var{flags}] if=@var{input} of=@var{output}
+@item dd [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] [seek=@var{blocks}] [conv=@var{convs}] [iflag=@var{flags}] [oflag=@var{flags}] if=@var{input} of=@var{output}
 
 Dd copies from @var{input} file to @var{output} file converting it from
 @var{fmt} format to @var{output_fmt} format.
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
new file mode 100755
index 0000000..749920d
--- /dev/null
+++ b/tests/qemu-iotests/165
@@ -0,0 +1,109 @@ 
+#! /bin/bash
+#
+# qemu-img dd test for conv option
+#
+# Copyright (C) 2016 Reda Sallahi
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+owner=fullmanet@gmail.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+status=1
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm -f "$TEST_IMG.out" "$TEST_IMG.out.dd"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+_supported_fmt raw
+_supported_proto file
+_supported_os Linux
+
+echo
+echo "== Creating image =="
+
+size=1M
+_make_test_img $size
+_check_test_img
+
+$QEMU_IO -c "write -P 0xa 215k 212k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "== Converting the image with dd with conv=excl =="
+
+$QEMU_IMG dd if="$TEST_IMG" of="$TEST_IMG.out" conv=excl,notrunc -O "$IMGFMT"
+
+TEST_IMG="$TEST_IMG.out" _check_test_img
+
+dd if="$TEST_IMG" of="$TEST_IMG.out.dd" conv=excl status=none
+
+echo
+echo "== Compare the images with qemu-img compare =="
+
+$QEMU_IMG compare "$TEST_IMG.out.dd" "$TEST_IMG.out"
+
+echo
+echo "== Converting the image with dd with conv=excl =="
+
+$QEMU_IMG dd if="$TEST_IMG" of="$TEST_IMG.out" conv=excl,notrunc \
+          -O "$IMGFMT" 2>&1 | sed -e "s#$TEST_DIR#TEST_DIR#g" \
+                                  -e "s#$IMGFMT#IMGFMT#g"
+
+echo
+echo "== Creating image =="
+
+_make_test_img $size
+_check_test_img
+
+$QEMU_IO -c "write -P 0xa 481k 329k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "== Converting the image with dd with conv=nocreat =="
+
+$QEMU_IMG dd if="$TEST_IMG" of="$TEST_IMG.out" conv=nocreat,notrunc \
+          -O "$IMGFMT"
+
+TEST_IMG="$TEST_IMG.out" _check_test_img
+
+dd if="$TEST_IMG" of="$TEST_IMG.out.dd" conv=nocreat status=none
+
+echo
+echo "== Compare the images with qemu-img compare =="
+
+$QEMU_IMG compare "$TEST_IMG.out.dd" "$TEST_IMG.out"
+
+rm -f "$TEST_IMG.out" "$TEST_IMG.out.dd"
+
+echo
+echo "== Converting the image with dd with conv=nocreat =="
+
+$QEMU_IMG dd if="$TEST_IMG" of="$TEST_IMG.out" conv=nocreat,notrunc \
+          -O "$IMGFMT" 2>&1 | sed -e "s#$TEST_DIR#TEST_DIR#g" \
+                                  -e "s#$IMGFMT#IMGFMT#g"
+
+echo
+echo "*** done"
+rm -f "$seq.full"
+status=0
diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
new file mode 100644
index 0000000..8db7f0a
--- /dev/null
+++ b/tests/qemu-iotests/165.out
@@ -0,0 +1,33 @@ 
+QA output created by 165
+
+== Creating image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+No errors were found on the image.
+wrote 217088/217088 bytes at offset 220160
+212 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Converting the image with dd with conv=excl ==
+No errors were found on the image.
+
+== Compare the images with qemu-img compare ==
+Images are identical.
+
+== Converting the image with dd with conv=excl ==
+qemu-img: failed to open 'TEST_DIR/t.IMGFMT.out': File exists
+
+== Creating image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+No errors were found on the image.
+wrote 336896/336896 bytes at offset 492544
+329 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Converting the image with dd with conv=nocreat ==
+No errors were found on the image.
+
+== Compare the images with qemu-img compare ==
+Images are identical.
+
+== Converting the image with dd with conv=nocreat ==
+qemu-img: Failed to open 'TEST_DIR/t.IMGFMT.out': No such file or directory
+
+*** done
diff --git a/tests/qemu-iotests/166 b/tests/qemu-iotests/166
new file mode 100755
index 0000000..409ffa8
--- /dev/null
+++ b/tests/qemu-iotests/166
@@ -0,0 +1,73 @@ 
+#! /bin/bash
+#
+# qemu-img dd test for conv=sparse
+#
+# Copyright (C) 2016 Reda Sallahi
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+owner=fullmanet@gmail.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+status=1
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm -f "$TEST_IMG.out" "$TEST_IMG.out.convert"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+echo
+echo "== Creating image =="
+
+size=1M
+_make_test_img $size
+_check_test_img
+
+$QEMU_IO -c "write -P 0 0 256k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "write -P 0xa 0 4k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "write -P 0xa 16k 2k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "== Converting the image with dd with conv=sparse =="
+
+$QEMU_IMG dd if="$TEST_IMG" of="$TEST_IMG.out" conv=sparse,notrunc \
+          -O "$IMGFMT"
+
+TEST_IMG="$TEST_IMG.out" _check_test_img
+
+$QEMU_IMG convert -S 512 "$TEST_IMG" "$TEST_IMG.out.convert" -O "$IMGFMT"
+
+echo
+echo "== Compare the images with qemu-img compare =="
+
+$QEMU_IMG compare "$TEST_IMG.out.convert" "$TEST_IMG.out"
+
+echo
+echo "*** done"
+rm -f "$seq.full"
+status=0
diff --git a/tests/qemu-iotests/166.out b/tests/qemu-iotests/166.out
new file mode 100644
index 0000000..012f545
--- /dev/null
+++ b/tests/qemu-iotests/166.out
@@ -0,0 +1,19 @@ 
+QA output created by 166
+
+== Creating image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+No errors were found on the image.
+wrote 262144/262144 bytes at offset 0
+256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 2048/2048 bytes at offset 16384
+2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Converting the image with dd with conv=sparse ==
+No errors were found on the image.
+
+== Compare the images with qemu-img compare ==
+Images are identical.
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 5ba1372..9ba23e5 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -164,3 +164,5 @@ 
 162 auto quick
 163 rw auto quick
 164 rw auto quick
+165 rw auto quick
+166 rw auto quick