[Qemu-devel] [PATCHv5] add qemu-img convert -n option (skip target volume creation)

[Qemu-devel] [PATCHv5] add qemu-img convert -n option (skip target volume creation)

[Alex Bligh]

Add a qemu-img convert -n option to skip target volume creation

Changes since v5:
* Fail conversion if the existing output image is smaller than the
  input image and the -n option is specified
* Use -n not -C
* Add a test routine

Alexandre Derumier (1):
  add qemu-img convert -n option (skip target volume creation)

 qemu-img-cmds.hx           |    4 +-
 qemu-img.c                 |   49 ++++++++++++++-------
 qemu-img.texi              |   15 ++++++-
 tests/qemu-iotests/060     |  101 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/060.out |   10 +++++
 tests/qemu-iotests/group   |    1 +
 6 files changed, 162 insertions(+), 18 deletions(-)
 create mode 100755 tests/qemu-iotests/060
 create mode 100644 tests/qemu-iotests/060.out

-- 
1.7.9.5

[Qemu-devel] [PATCHv5] add qemu-img convert -n option (skip target volume creation)

[Alex Bligh]

From: Alexandre Derumier <aderumier@odiso.com>

Add a -n option to skip volume creation on qemu-img convert.
This is useful for targets such as rbd / ceph, where the
target volume may already exist; we cannot always rely on
qemu-img convert to create the image, as dependent on the
output format, there may be parameters which are not possible
to specify through the qemu-img convert command line.

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
Signed-off-by: Alex Bligh <alex@alex.org.uk>
---
 qemu-img-cmds.hx           |    4 +-
 qemu-img.c                 |   49 ++++++++++++++-------
 qemu-img.texi              |   15 ++++++-
 tests/qemu-iotests/060     |  101 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/060.out |   10 +++++
 tests/qemu-iotests/group   |    1 +
 6 files changed, 162 insertions(+), 18 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 4ca7e95..2f6d579 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -34,9 +34,9 @@ STEXI
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [-c] [-p] [-q] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename")
+    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-O output_fmt] [-o options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] output_filename")
 STEXI
-@item convert [-c] [-p] [-q] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("info", img_info,
diff --git a/qemu-img.c b/qemu-img.c
index b9a848d..8e60ced 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -103,6 +103,8 @@ static void help(void)
            "  '-S' indicates the consecutive number of bytes that must contain only zeros\n"
            "       for qemu-img to create a sparse image during conversion\n"
            "  '--output' takes the format in which the output must be done (human or json)\n"
+           "  '-n' skips the target volume creation (useful if the volume is created\n"
+           "       prior to running qemu-img)\n"
            "\n"
            "Parameters to check subcommand:\n"
            "  '-r' tries to repair any inconsistencies that are found during the check.\n"
@@ -1116,7 +1118,8 @@ out3:
 
 static int img_convert(int argc, char **argv)
 {
-    int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors;
+    int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size,
+        cluster_sectors, skip_create;
     int progress = 0, flags;
     const char *fmt, *out_fmt, *cache, *out_baseimg, *out_filename;
     BlockDriver *drv, *proto_drv;
@@ -1139,8 +1142,9 @@ static int img_convert(int argc, char **argv)
     cache = "unsafe";
     out_baseimg = NULL;
     compress = 0;
+    skip_create = 0;
     for(;;) {
-        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:q");
+        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:qn");
         if (c == -1) {
             break;
         }
@@ -1161,6 +1165,9 @@ static int img_convert(int argc, char **argv)
         case 'c':
             compress = 1;
             break;
+        case 'n':
+            skip_create = 1;
+            break;
         case 'e':
             error_report("option -e is deprecated, please use \'-o "
                   "encryption\' instead!");
@@ -1329,20 +1336,22 @@ static int img_convert(int argc, char **argv)
         }
     }
 
-    /* Create the new image */
-    ret = bdrv_create(drv, out_filename, param);
-    if (ret < 0) {
-        if (ret == -ENOTSUP) {
-            error_report("Formatting not supported for file format '%s'",
-                         out_fmt);
-        } else if (ret == -EFBIG) {
-            error_report("The image size is too large for file format '%s'",
-                         out_fmt);
-        } else {
-            error_report("%s: error while converting %s: %s",
-                         out_filename, out_fmt, strerror(-ret));
+    if (!skip_create) {
+        /* Create the new image */
+        ret = bdrv_create(drv, out_filename, param);
+        if (ret < 0) {
+            if (ret == -ENOTSUP) {
+                error_report("Formatting not supported for file format '%s'",
+                             out_fmt);
+            } else if (ret == -EFBIG) {
+                error_report("The image size is too large for file format '%s'",
+                             out_fmt);
+            } else {
+                error_report("%s: error while converting %s: %s",
+                             out_filename, out_fmt, strerror(-ret));
+            }
+            goto out;
         }
-        goto out;
     }
 
     flags = BDRV_O_RDWR;
@@ -1363,6 +1372,16 @@ static int img_convert(int argc, char **argv)
     bdrv_get_geometry(bs[0], &bs_sectors);
     buf = qemu_blockalign(out_bs, IO_BUF_SIZE);
 
+    if (skip_create) {
+        uint64_t out_bs_sectors = 0;
+        bdrv_get_geometry(out_bs, &out_bs_sectors);
+        if (out_bs_sectors < total_sectors) {
+            error_report("output file is smaller than input file");
+            ret = -1;
+            goto out;
+        }
+    }
+
     if (compress) {
         ret = bdrv_get_info(out_bs, &bdi);
         if (ret < 0) {
diff --git a/qemu-img.texi b/qemu-img.texi
index 69f1bda..ad45a6d 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -96,6 +96,14 @@ Second image format
 Strict mode - fail on on different image size or sector allocation
 @end table
 
+Parameters to convert subcommand:
+
+@table @option
+
+@item -n
+Skip the creation of the target volume
+@end table
+
 Command description:
 
 @table @option
@@ -171,7 +179,7 @@ Error on reading data
 
 @end table
 
-@item convert [-c] [-p] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [-c] [-p] [-n] [-f @var{fmt}] [-t @var{cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 
 Convert the disk image @var{filename} or a snapshot @var{snapshot_name} to disk image @var{output_filename}
 using format @var{output_fmt}. It can be optionally compressed (@code{-c}
@@ -190,6 +198,11 @@ created as a copy on write image of the specified base image; the
 @var{backing_file} should have the same content as the input's base image,
 however the path, image format, etc may differ.
 
+If the @code{-n} option is specified, the target volume creation will be
+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 info [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
 
 Give information about the disk image @var{filename}. Use it in
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
new file mode 100755
index 0000000..bd32710
--- /dev/null
+++ b/tests/qemu-iotests/060
@@ -0,0 +1,101 @@
+#!/bin/bash
+#
+# test of qemu-img convert -n - convert without creation
+#
+# Copyright (C) 2009 Red Hat, Inc.
+#
+# 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/>.
+#
+
+# creator
+owner=alex@alex.org.uk
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+	rm -f $TEST_IMG.orig $TEST_IMG.raw $TEST_IMG.raw2
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+# much of this could be generic for any format supporting compression.
+_supported_fmt qcow qcow2 vmdk qed raw
+_supported_proto generic
+_supported_os Linux
+
+TEST_OFFSETS="0 4294967296"
+TEST_OPS="writev read write readv"
+CLUSTER_SIZE=4096
+
+_make_test_img 4M
+
+echo "== Testing conversion with -n fails with no target file =="
+# check .orig file does not exist
+rm -f $TEST_IMG.orig
+if $QEMU_IMG convert -f $IMGFMT -O $IMGFMT -n $TEST_IMG $TEST_IMG.orig >/dev/null 2>&1; then
+    exit 1
+fi
+
+echo "== Testing conversion with -n succeeds with a target file =="
+rm -f $TEST_IMG.orig
+cp $TEST_IMG $TEST_IMG.orig
+if ! $QEMU_IMG convert -f $IMGFMT -O $IMGFMT -n $TEST_IMG $TEST_IMG.orig ; then
+    exit 1
+fi
+
+echo "== Testing conversion to raw is the same after conversion with -n =="
+# compare the raw files
+if ! $QEMU_IMG convert -f $IMGFMT -O raw $TEST_IMG $TEST_IMG.raw1 ; then
+    exit 1
+fi
+
+if ! $QEMU_IMG convert -f $IMGFMT -O raw $TEST_IMG.orig $TEST_IMG.raw2 ; then
+    exit 1
+fi
+
+if ! cmp $TEST_IMG.raw1 $TEST_IMG.raw2 ; then
+    exit 1
+fi
+
+echo "== Testing conversion back to original format =="
+if ! $QEMU_IMG convert -f raw -O $IMGFMT -n $TEST_IMG.raw2 $TEST_IMG ; then
+    exit 1
+fi
+_check_test_img
+
+echo "== Testing conversion to a smaller file fails =="
+rm -f $TEST_IMG.orig
+mv $TEST_IMG $TEST_IMG.orig
+_make_test_img 2M
+if $QEMU_IMG convert -f $IMGFMT -O $IMGFMT -n $TEST_IMG.orig $TEST_IMG >/dev/null 2>&1; then
+    exit 1
+fi
+
+rm -f $TEST_IMG.orig $TEST_IMG.raw $TEST_IMG.raw2
+
+echo "*** done"
+rm -f $seq.full
+status=0
+exit 0
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
new file mode 100644
index 0000000..29004f5
--- /dev/null
+++ b/tests/qemu-iotests/060.out
@@ -0,0 +1,10 @@
+QA output created by 060
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304
+== Testing conversion with -n fails with no target file ==
+== Testing conversion with -n succeeds with a target file ==
+== Testing conversion to raw is the same after conversion with -n ==
+== Testing conversion back to original format ==
+No errors were found on the image.
+== Testing conversion to a smaller file fails ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2097152
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 43c05d6..0845eb5 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -64,3 +64,4 @@
 055 rw auto
 056 rw auto backing
 059 rw auto
+060 rw auto
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv5] add qemu-img convert -n option (skip target volume creation)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]

On 08/26/2013 03:12 AM, Alex Bligh wrote:
> From: Alexandre Derumier <aderumier@odiso.com>
> 
> Add a -n option to skip volume creation on qemu-img convert.
> This is useful for targets such as rbd / ceph, where the
> target volume may already exist; we cannot always rely on
> qemu-img convert to create the image, as dependent on the
> output format, there may be parameters which are not possible
> to specify through the qemu-img convert command line.
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> Signed-off-by: Alex Bligh <alex@alex.org.uk>
> ---

>  static int img_convert(int argc, char **argv)
>  {
> -    int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors;
> +    int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size,
> +        cluster_sectors, skip_create;

Pre-existing usage, but this new variable is used only as a bool, so it
might be better to type it (and others like it) correctly...

>      int progress = 0, flags;
>      const char *fmt, *out_fmt, *cache, *out_baseimg, *out_filename;
>      BlockDriver *drv, *proto_drv;
> @@ -1139,8 +1142,9 @@ static int img_convert(int argc, char **argv)
>      cache = "unsafe";
>      out_baseimg = NULL;
>      compress = 0;
> +    skip_create = 0;

...which would mean using 'false' instead of '0' here, and so forth.  As
at least 'compress' is also impacted, I'm okay deferring the type
cleanup to a separate patch.

>      for(;;) {
> -        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:q");
> +        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:qn");

The order here...

>          if (c == -1) {
>              break;
>          }
> @@ -1161,6 +1165,9 @@ static int img_convert(int argc, char **argv)
>          case 'c':
>              compress = 1;
>              break;
> +        case 'n':
> +            skip_create = 1;
> +            break;

...and the order of the case statements, are unrelated.  Which makes
maintenance a bit harder.  Personally, I like to keep my optstring in
case-insensitive order, then my case statements in the same order.  But
that's cosmetic, it doesn't affect the correctness of the patch.

> +++ b/tests/qemu-iotests/060
> @@ -0,0 +1,101 @@
> +#!/bin/bash
> +#
> +# test of qemu-img convert -n - convert without creation
> +#
> +# Copyright (C) 2009 Red Hat, Inc.

Where have you been the last 4 years?  I could understand a range of
years, if this test borrows significantly from another file that old,
but I suspect that just 2013 is probably more accurate.

Fix that, and you can add:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCHv5] add qemu-img convert -n option (skip target volume creation)

[Alex Bligh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Eric,

On 30 Aug 2013, at 18:31, Eric Blake wrote:
>> 
>>     for(;;) {
>> -        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:q");
>> +        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:qn");
> 
> The order here...
> 
>>         if (c == -1) {
>>             break;
>>         }
>> @@ -1161,6 +1165,9 @@ static int img_convert(int argc, char **argv)
>>         case 'c':
>>             compress = 1;
>>             break;
>> +        case 'n':
>> +            skip_create = 1;
>> +            break;
> 
> ...and the order of the case statements, are unrelated.  Which makes
> maintenance a bit harder.  Personally, I like to keep my optstring in
> case-insensitive order, then my case statements in the same order.  But
> that's cosmetic, it doesn't affect the correctness of the patch.

I had also thought the order was unrelated, but closer inspection shows
that the ordering is 'similar'. If I'm going to roll another one, I'll
stick case 'n' after case 'q' and at least not make it any worse.

>> 
>> +++ b/tests/qemu-iotests/060
>> @@ -0,0 +1,101 @@
>> +#!/bin/bash
>> +#
>> +# test of qemu-img convert -n - convert without creation
>> +#
>> +# Copyright (C) 2009 Red Hat, Inc.
> 
> Where have you been the last 4 years?  I could understand a range of
> years, if this test borrows significantly from another file that old,
> but I suspect that just 2013 is probably more accurate.

I've been not at Redhat for any of it!

I wasn't too sure what to do with the copyright string. The body of
the code was copied from another test (058 if I remember rightly)
which had the above copyright string which is presumably that old.
Redhat don't have any copyright in the stuff I added so there is
no (c) Redhat newer than (presumably) 2009 (I'm presuming qemu
doesn't require contributor assignment). My copyright would be 2013.
But removing Redhat's (c) or altering it did not seem right.

I'm really not bothered about having my own string in there
particularly for such a meagre bit of code, but I suppose most
accurate would be:

  (c) 2009 Red Hat, Inc.
  (c) 2013 Alex Bligh

?

- -- 
Alex Bligh




-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.20 (Darwin)
Comment: GPGTools - https://gpgtools.org

iQIcBAEBCgAGBQJSIODKAAoJEBPm5K7i9iORZCUP/j+XQNUkc9xbolDNAkK6S3AD
xSmdqBtuEbnp14C01w9CDTV+ncUyPCYIByF74XjNT9R6tV2pcrfWFaEJVsssBEJd
sy9fNJPngz+x4VHLNkFT4kM8+QUwFfd0sNkW5nGvxJeJkjC76cBaMaHlFEePBEK0
7JDwXyjm+9upn6TVFu5rdp9424wia0gvDPFTpSm/ocAYO55m0C7GUdPET2qTqsLa
gqJeNgYQOIts1NtaLalghRTbdvePJjFPEiTkS6gwgew5SO1cKslteEpnC0Sq6I+E
NT1tYYlKsQmdGoXc31CIuQxbWvaumh0Tb5Q6cmRDQ9obTKZZmoDvTuZXZrviCfDg
3AGyClY703Wemzcz1cjzSjubJ9d43f6IqcY+NkQPyfvIh+XPjE1QMjMe/sd2so7h
yybxYZpLd/oz1g5jFRD+l0nJ338iVOfv8wrLy4Xt99lH/rOT4OgsY8WXT3Hoyv6P
IUaijGVbamh6GGas4CsBbZdOG7zK+Y9/sGhdKdjgFrAZMzlzpTb+9Q6TX12exBNS
k3SYapGSns3ZHmo2De8tZOOuOZk5eiZeWk90PuXp3myEN2snwUXp68E0uMyPsiBO
9FIQtCskH/xNf0zqAfPxC9CQ3jCCuN3fe0E+cs5sstMpj5+bkk5Ie1zWPsA30L/w
1cS/ogF4A3Wbz8+ZOsDG
=iSLE
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCHv5] add qemu-img convert -n option (skip target volume creation)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1713 bytes --]

On 08/30/2013 12:13 PM, Alex Bligh wrote:

>>> +# test of qemu-img convert -n - convert without creation
>>> +#
>>> +# Copyright (C) 2009 Red Hat, Inc.
> 
>> Where have you been the last 4 years?  I could understand a range of
>> years, if this test borrows significantly from another file that old,
>> but I suspect that just 2013 is probably more accurate.
> 
> I've been not at Redhat for any of it!
> 
> I wasn't too sure what to do with the copyright string. The body of
> the code was copied from another test (058 if I remember rightly)
> which had the above copyright string which is presumably that old.

If so, and if I reviewed that patch, shame on me for not noticing the
stale year on that test :)

> Redhat don't have any copyright in the stuff I added so there is
> no (c) Redhat newer than (presumably) 2009 (I'm presuming qemu
> doesn't require contributor assignment). My copyright would be 2013.

Your own work is non-trivial - by virtue of the fact that you list:

+owner=alex@alex.org.uk

you are stating that there is enough new content comprising this test
that you feel comfortable being listed as the point of contact for the file.

> But removing Redhat's (c) or altering it did not seem right.

Indeed, since you are copying a substantial amount of template from Red
Hat's copyright.

> 
> I'm really not bothered about having my own string in there
> particularly for such a meagre bit of code, but I suppose most
> accurate would be:
> 
>   (c) 2009 Red Hat, Inc.
>   (c) 2013 Alex Bligh

Yes, that looks like the best approach.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCHv5] add qemu-img convert -n option (skip target volume creation)

[Alex Bligh]

--On 30 August 2013 12:55:29 -0600 Eric Blake <eblake@redhat.com> wrote:

> Yes, that looks like the best approach.

Thanks - done in v6 and added your reviewed-by line

-- 
Alex Bligh