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

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

[Alex Bligh]

From: Alexandre Derumier <aderumier@odiso.com>

Add a -C 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       |   39 ++++++++++++++++++++++++---------------
 qemu-img.texi    |   15 ++++++++++++++-
 3 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 4ca7e95..74ced81 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] [-C] [-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] [-C] [-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..146909b 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"
+           "  '-C' 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, skipcreate;
     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;
+    skipcreate = 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:qC");
         if (c == -1) {
             break;
         }
@@ -1161,6 +1165,9 @@ static int img_convert(int argc, char **argv)
         case 'c':
             compress = 1;
             break;
+        case 'C':
+            skipcreate = 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 (!skipcreate) {
+        /* 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;
diff --git a/qemu-img.texi b/qemu-img.texi
index 69f1bda..9e5ba36 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 -C
+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] [-C] [-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{-C} 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
-- 
1.7.9.5

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

[Stefan Hajnoczi]

On Mon, Aug 12, 2013 at 12:41:50PM +0100, Alex Bligh wrote:
> From: Alexandre Derumier <aderumier@odiso.com>
> 
> Add a -C 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       |   39 ++++++++++++++++++++++++---------------
>  qemu-img.texi    |   15 ++++++++++++++-
>  3 files changed, 40 insertions(+), 18 deletions(-)

Looks good but please include a new qemu-iotest test case that checks:

1. Error if the target volume does not exist.

2. Success if a correctly sized target volume exists.

3. ?? if an incorrectly sized target volume exists.

...and anything else you feel is worth testing.

I recommend keeping the test volume size small so the test case can
execute quickly.  1 MB should be fine for raw or qcow2 images.

Stefan

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

[Paolo Bonzini]

Il 22/08/2013 13:45, Stefan Hajnoczi ha scritto:
> On Mon, Aug 12, 2013 at 12:41:50PM +0100, Alex Bligh wrote:
>> > From: Alexandre Derumier <aderumier@odiso.com>
>> > 
>> > Add a -C 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       |   39 ++++++++++++++++++++++++---------------
>> >  qemu-img.texi    |   15 ++++++++++++++-
>> >  3 files changed, 40 insertions(+), 18 deletions(-)
> Looks good but please include a new qemu-iotest test case that checks:
> 
> 1. Error if the target volume does not exist.
> 
> 2. Success if a correctly sized target volume exists.
> 
> 3. ?? if an incorrectly sized target volume exists.
> 
> ...and anything else you feel is worth testing.
> 
> I recommend keeping the test volume size small so the test case can
> execute quickly.  1 MB should be fine for raw or qcow2 images.

Also, this is the same as some HMP commands' "-n" option (live
snapshots, mirroring, backup) so I suggest to use that name.

Paolo

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

[Alex Bligh]

Paolo,

On 22 Aug 2013, at 13:35, Paolo Bonzini wrote:

> Also, this is the same as some HMP commands' "-n" option (live
> snapshots, mirroring, backup) so I suggest to use that name.

You mean -n instead of -C? Sure I can do that, but is that
something you feel strongly about? I am aware there are a number
of people who have been using the patch with -C for some time.

-- 
Alex Bligh

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

[Paolo Bonzini]

Il 22/08/2013 20:03, Alex Bligh ha scritto:
> Paolo,
> 
> On 22 Aug 2013, at 13:35, Paolo Bonzini wrote:
> 
>> Also, this is the same as some HMP commands' "-n" option (live
>> snapshots, mirroring, backup) so I suggest to use that name.
> 
> You mean -n instead of -C? Sure I can do that, but is that
> something you feel strongly about? I am aware there are a number
> of people who have been using the patch with -C for some time.

I'll be fine if Stefan and Kevin override me, but yes, I think
consistency is important.  qemu-img has offline functionality equivalent
to block jobs, and it is useful to keep qemu-img subcommands as
consistent as possible between them and with the corresponding HMP
commands.  convert is the equivalent of the online mirroring block job.

Paolo

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

[Alex Bligh]

--On 22 August 2013 21:30:41 +0200 Paolo Bonzini <pbonzini@redhat.com> 
wrote:

>>> Also, this is the same as some HMP commands' "-n" option (live
>>> snapshots, mirroring, backup) so I suggest to use that name.
>>
>> You mean -n instead of -C? Sure I can do that, but is that
>> something you feel strongly about? I am aware there are a number
>> of people who have been using the patch with -C for some time.
>
> I'll be fine if Stefan and Kevin override me, but yes, I think
> consistency is important.  qemu-img has offline functionality equivalent
> to block jobs, and it is useful to keep qemu-img subcommands as
> consistent as possible between them and with the corresponding HMP
> commands.  convert is the equivalent of the online mirroring block job.

I note -f in hmp means 'whole disk' whereas in qemu-img it means 'format'.
A quick scan suggests there are no other flags that do similar things.
So this would be the first flag that is consistent I think!

-- 
Alex Bligh

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

[Eric Blake]

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

On 08/22/2013 12:03 PM, Alex Bligh wrote:
> Paolo,
> 
> On 22 Aug 2013, at 13:35, Paolo Bonzini wrote:
> 
>> Also, this is the same as some HMP commands' "-n" option (live
>> snapshots, mirroring, backup) so I suggest to use that name.
> 
> You mean -n instead of -C? Sure I can do that, but is that
> something you feel strongly about? I am aware there are a number
> of people who have been using the patch with -C for some time.

Anyone using a non-upstream patch should already be prepared for
upstream to take a different course.  Such a user can add -C as a
synonym for -n (at least, until upstream DOES add a -C with different
semantics), or rewrite their scripts.

I too would like to reuse -n for consistency.

-- 
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] [PATCHv4] add qemu-img convert -C option (skip target volume creation)

[Stefan Hajnoczi]

On Thu, Aug 22, 2013 at 07:03:41PM +0100, Alex Bligh wrote:
> On 22 Aug 2013, at 13:35, Paolo Bonzini wrote:
> 
> > Also, this is the same as some HMP commands' "-n" option (live
> > snapshots, mirroring, backup) so I suggest to use that name.
> 
> You mean -n instead of -C? Sure I can do that, but is that
> something you feel strongly about? I am aware there are a number
> of people who have been using the patch with -C for some time.

Sharing the -n flag with other commands is better than introducing a new
flag.

Stefan

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

[Alex Bligh]

On 22 Aug 2013, at 12:45, Stefan Hajnoczi wrote:

> Looks good but please include a new qemu-iotest test case that checks:
> 
> 1. Error if the target volume does not exist.

Can do

> 2. Success if a correctly sized target volume exists.
> 
> 3. ?? if an incorrectly sized target volume exists.

Good catch re size.

The behaviour of the current code appears to be as follows
(Alexandre's code not mine):

a) if the target volume size > the converted volume size, convert
   leaving the remaining data on the target volume as is. This
   is I believe useful, as on (e.g.) rbd, the target volume size
   may be larger than required due to rounding requirements.

b) if the target volume size < the converted volume size, convert
   truncates it but does not error. I'm torn between whether this
   continue to do exactly as it asked, attempt to expand the
   volume, or error. These all seem reasonably easy (the expand
   option presumably being a call to bdrv_truncate).

Before we get to testing, I'm guessing we should establish
whether (a) is correct (I think yes), and what the correct
behaviour is for (b).

-- 
Alex Bligh

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

[Stefan Hajnoczi]

On Thu, Aug 22, 2013 at 08:27:12PM +0100, Alex Bligh wrote:
> 
> On 22 Aug 2013, at 12:45, Stefan Hajnoczi wrote:
> The behaviour of the current code appears to be as follows
> (Alexandre's code not mine):
> 
> a) if the target volume size > the converted volume size, convert
>    leaving the remaining data on the target volume as is. This
>    is I believe useful, as on (e.g.) rbd, the target volume size
>    may be larger than required due to rounding requirements.

Seems ok.

> b) if the target volume size < the converted volume size, convert
>    truncates it but does not error. I'm torn between whether this
>    continue to do exactly as it asked, attempt to expand the
>    volume, or error. These all seem reasonably easy (the expand
>    option presumably being a call to bdrv_truncate).

Silently truncating can be a problem, e.g. the user deletes the original
file after conversion completes and later discovers not all data was
copied.

I think we should fail here.

Stefan

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

[Alex Bligh]

On 26 Aug 2013, at 09:03, Stefan Hajnoczi wrote:

> Silently truncating can be a problem, e.g. the user deletes the original
> file after conversion completes and later discovers not all data was
> copied.
> 
> I think we should fail here.

Fixed in v5 (just sent), together with using -n not -C (obviously
I'm fighting a losing battle there).

-- 
Alex Bligh