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

[Eric Blake <eblake@redhat.com>]

[-- 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 --]