[Qemu-devel] [PATCH v3 0/7] Cleanup qemu-img code

[Qemu-devel] [PATCH v3 0/7] Cleanup qemu-img code

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

These patches applies a number of cleanups to qemu-img.c as well as a
minor bug in qemu-malloc.c. 

The handling of block help printing is moved to shared code, which
allows the "?" detection to happen early in the parsing, instead of
half way down img_create() and img_convert(). I would like to see this
happen as I would like to pull some of the code out of img_create()
and into block.c so it can be shared with qemu and qemu-img.

In addition there is a couple of patches to clean up the error
handling in qemu-img.c and make it more consistent.

The formatting patch is solely because the last patch wanted to
change code next to the badly formatted code, and I didn't want to
pollute the patch with the formatting fixed.

The seventh patch fixes qemu-img to exit on detection of unknown
options instead of continuing with a potentially wrong set of
arguments.

v3 applies a number of changes discussed on irc and email. This is the
grow to seven from three patches series.

Cheers,
Jes

Jes Sorensen (7):
  Add missing tracing to qemu_mallocz()
  Use qemu_mallocz() instead of calloc() in img_convert()
  img_convert(): Only try to free bs[] entries if bs is valid.
  Make error handling more consistent in img_create() and img_resize()
  Consolidate printing of block driver options
  Fix formatting and missing braces in qemu-img.c
  Fail if detecting an unknown option

 qemu-img.c    |  162 +++++++++++++++++++++++++++++++++++++++-----------------
 qemu-malloc.c |    5 ++-
 2 files changed, 117 insertions(+), 50 deletions(-)

-- 
1.7.3.2

[Qemu-devel] [PATCH 1/7] Add missing tracing to qemu_mallocz()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qemu-malloc.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/qemu-malloc.c b/qemu-malloc.c
index 28fb05a..b9b3851 100644
--- a/qemu-malloc.c
+++ b/qemu-malloc.c
@@ -64,10 +64,13 @@ void *qemu_realloc(void *ptr, size_t size)
 
 void *qemu_mallocz(size_t size)
 {
+    void *ptr;
     if (!size && !allow_zero_malloc()) {
         abort();
     }
-    return qemu_oom_check(calloc(1, size ? size : 1));
+    ptr = qemu_oom_check(calloc(1, size ? size : 1));
+    trace_qemu_malloc(size, ptr);
+    return ptr;
 }
 
 char *qemu_strdup(const char *str)
-- 
1.7.3.2

[Qemu-devel] Re: [PATCH 1/7] Add missing tracing to qemu_mallocz()

[Stefan Hajnoczi]

On Mon, Dec 06, 2010 at 03:25:34PM +0100, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  qemu-malloc.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] [PATCH 2/7] Use qemu_mallocz() instead of calloc() in img_convert()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qemu-img.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index fa77ac0..eca99c4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -699,11 +699,7 @@ static int img_convert(int argc, char **argv)
         return 1;
     }
         
-    bs = calloc(bs_n, sizeof(BlockDriverState *));
-    if (!bs) {
-        error("Out of memory");
-        return 1;
-    }
+    bs = qemu_mallocz(bs_n * sizeof(BlockDriverState *));
 
     total_sectors = 0;
     for (bs_i = 0; bs_i < bs_n; bs_i++) {
@@ -983,7 +979,7 @@ out:
             bdrv_delete(bs[bs_i]);
         }
     }
-    free(bs);
+    qemu_free(bs);
     if (ret) {
         return 1;
     }
-- 
1.7.3.2

[Qemu-devel] Re: [PATCH 2/7] Use qemu_mallocz() instead of calloc() in img_convert()

[Stefan Hajnoczi]

On Mon, Dec 06, 2010 at 03:25:35PM +0100, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  qemu-img.c |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] [PATCH 3/7] img_convert(): Only try to free bs[] entries if bs is valid.

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

This allows for jumping to 'out:' consistently for error exit.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qemu-img.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index eca99c4..aded72d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -696,7 +696,8 @@ static int img_convert(int argc, char **argv)
 
     if (bs_n > 1 && out_baseimg) {
         error("-B makes no sense when concatenating multiple input images");
-        return 1;
+        ret = -1;
+        goto out;
     }
         
     bs = qemu_mallocz(bs_n * sizeof(BlockDriverState *));
@@ -974,12 +975,14 @@ out:
     if (out_bs) {
         bdrv_delete(out_bs);
     }
-    for (bs_i = 0; bs_i < bs_n; bs_i++) {
-        if (bs[bs_i]) {
-            bdrv_delete(bs[bs_i]);
+    if (bs) {
+        for (bs_i = 0; bs_i < bs_n; bs_i++) {
+            if (bs[bs_i]) {
+                bdrv_delete(bs[bs_i]);
+            }
         }
+        qemu_free(bs);
     }
-    qemu_free(bs);
     if (ret) {
         return 1;
     }
-- 
1.7.3.2

[Qemu-devel] Re: [PATCH 3/7] img_convert(): Only try to free bs[] entries if bs is valid.

[Stefan Hajnoczi]

On Mon, Dec 06, 2010 at 03:25:36PM +0100, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> This allows for jumping to 'out:' consistently for error exit.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  qemu-img.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] [PATCH 4/7] Make error handling more consistent in img_create() and img_resize()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qemu-img.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index aded72d..7f4939e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -314,13 +314,15 @@ static int img_create(int argc, char **argv)
     drv = bdrv_find_format(fmt);
     if (!drv) {
         error("Unknown file format '%s'", fmt);
-        return 1;
+        ret = -1;
+        goto out;
     }
 
     proto_drv = bdrv_find_protocol(filename);
     if (!proto_drv) {
         error("Unknown protocol '%s'", filename);
-        return 1;
+        ret = -1;
+        goto out;
     }
 
     create_options = append_option_parameters(create_options,
@@ -1483,14 +1485,16 @@ static int img_resize(int argc, char **argv)
     param = parse_option_parameters("", resize_options, NULL);
     if (set_option_parameter(param, BLOCK_OPT_SIZE, size)) {
         /* Error message already printed when size parsing fails */
-        exit(1);
+        ret = -1;
+        goto out;
     }
     n = get_option_parameter(param, BLOCK_OPT_SIZE)->value.n;
     free_option_parameters(param);
 
     bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR);
     if (!bs) {
-        return 1;
+        ret = -1;
+        goto out;
     }
 
     if (relative) {
-- 
1.7.3.2

[Qemu-devel] Re: [PATCH 4/7] Make error handling more consistent in img_create() and img_resize()

[Stefan Hajnoczi]

On Mon, Dec 06, 2010 at 03:25:37PM +0100, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  qemu-img.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] Re: [PATCH 4/7] Make error handling more consistent in img_create() and img_resize()

[Kevin Wolf]

Am 06.12.2010 15:25, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  qemu-img.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index aded72d..7f4939e 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -314,13 +314,15 @@ static int img_create(int argc, char **argv)
>      drv = bdrv_find_format(fmt);
>      if (!drv) {
>          error("Unknown file format '%s'", fmt);
> -        return 1;
> +        ret = -1;
> +        goto out;
>      }
>  
>      proto_drv = bdrv_find_protocol(filename);
>      if (!proto_drv) {
>          error("Unknown protocol '%s'", filename);
> -        return 1;
> +        ret = -1;
> +        goto out;
>      }
>  
>      create_options = append_option_parameters(create_options,
> @@ -1483,14 +1485,16 @@ static int img_resize(int argc, char **argv)
>      param = parse_option_parameters("", resize_options, NULL);
>      if (set_option_parameter(param, BLOCK_OPT_SIZE, size)) {
>          /* Error message already printed when size parsing fails */
> -        exit(1);
> +        ret = -1;
> +        goto out;

bs isn't initialized here, so the bdrv_delete(bs) after out: will crash.

>      }
>      n = get_option_parameter(param, BLOCK_OPT_SIZE)->value.n;
>      free_option_parameters(param);
>  
>      bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR);
>      if (!bs) {
> -        return 1;
> +        ret = -1;
> +        goto out;
>      }

Same here.

Heh, wanted to try it out to be sure, but the compiler notices that, so
it doesn't even build:

cc1: warnings being treated as errors
qemu-img.c: In function 'img_resize':
qemu-img.c:1497: error: 'bs' may be used uninitialized in this function

Kevin

[Qemu-devel] Re: [PATCH 4/7] Make error handling more consistent in img_create() and img_resize()

[Jes Sorensen]

On 12/06/10 16:25, Kevin Wolf wrote:
>>      }
>>      n = get_option_parameter(param, BLOCK_OPT_SIZE)->value.n;
>>      free_option_parameters(param);
>>  
>>      bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR);
>>      if (!bs) {
>> -        return 1;
>> +        ret = -1;
>> +        goto out;
>>      }
> 
> Same here.
> 
> Heh, wanted to try it out to be sure, but the compiler notices that, so
> it doesn't even build:

Grrrrr I am an idiot!

Sorry about the noise, I was sure I had tested that last change. Fix
coming up in a few minutes.

Cheers,
Jes

[Qemu-devel] [PATCH 5/7] Consolidate printing of block driver options

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

This consolidates the printing of block driver options in
print_block_option_help() which is called from both img_create() and
img_convert().

This allows for the "?" detection to be done just after the parsing of
options and the filename, instead of half way down the codepath of
these functions.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qemu-img.c |   46 +++++++++++++++++++++++++++++++++++++---------
 1 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 7f4939e..c7d0ca8 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -188,6 +188,33 @@ static int read_password(char *buf, int buf_size)
 }
 #endif
 
+static int print_block_option_help(const char *filename, const char *fmt)
+{
+    BlockDriver *drv, *proto_drv;
+    QEMUOptionParameter *create_options = NULL;
+
+    /* Find driver and parse its options */
+    drv = bdrv_find_format(fmt);
+    if (!drv) {
+        error("Unknown file format '%s'", fmt);
+        return 1;
+    }
+
+    proto_drv = bdrv_find_protocol(filename);
+    if (!proto_drv) {
+        error("Unknown protocol '%s'", filename);
+        return 1;
+    }
+
+    create_options = append_option_parameters(create_options,
+                                              drv->create_options);
+    create_options = append_option_parameters(create_options,
+                                              proto_drv->create_options);
+    print_option_help(create_options);
+    free_option_parameters(create_options);
+    return 0;
+}
+
 static BlockDriverState *bdrv_new_open(const char *filename,
                                        const char *fmt,
                                        int flags)
@@ -310,6 +337,11 @@ static int img_create(int argc, char **argv)
         help();
     filename = argv[optind++];
 
+    if (options && !strcmp(options, "?")) {
+        ret = print_block_option_help(filename, fmt);
+        goto out;
+    }
+
     /* Find driver and parse its options */
     drv = bdrv_find_format(fmt);
     if (!drv) {
@@ -330,11 +362,6 @@ static int img_create(int argc, char **argv)
     create_options = append_option_parameters(create_options,
                                               proto_drv->create_options);
 
-    if (options && !strcmp(options, "?")) {
-        print_option_help(create_options);
-        goto out;
-    }
-
     /* Create parameter list with default values */
     param = parse_option_parameters("", create_options, param);
     set_option_parameter_int(param, BLOCK_OPT_SIZE, -1);
@@ -696,6 +723,11 @@ static int img_convert(int argc, char **argv)
 
     out_filename = argv[argc - 1];
 
+    if (options && !strcmp(options, "?")) {
+        ret = print_block_option_help(out_filename, out_fmt);
+        goto out;
+    }
+
     if (bs_n > 1 && out_baseimg) {
         error("-B makes no sense when concatenating multiple input images");
         ret = -1;
@@ -748,10 +780,6 @@ static int img_convert(int argc, char **argv)
                                               drv->create_options);
     create_options = append_option_parameters(create_options,
                                               proto_drv->create_options);
-    if (options && !strcmp(options, "?")) {
-        print_option_help(create_options);
-        goto out;
-    }
 
     if (options) {
         param = parse_option_parameters(options, create_options, param);
-- 
1.7.3.2

[Qemu-devel] Re: [PATCH 5/7] Consolidate printing of block driver options

[Stefan Hajnoczi]

On Mon, Dec 06, 2010 at 03:25:38PM +0100, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> This consolidates the printing of block driver options in
> print_block_option_help() which is called from both img_create() and
> img_convert().
> 
> This allows for the "?" detection to be done just after the parsing of
> options and the filename, instead of half way down the codepath of
> these functions.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  qemu-img.c |   46 +++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 37 insertions(+), 9 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] [PATCH 6/7] Fix formatting and missing braces in qemu-img.c

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 qemu-img.c |   77 +++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index c7d0ca8..d812db0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -305,8 +305,9 @@ static int img_create(int argc, char **argv)
     flags = 0;
     for(;;) {
         c = getopt(argc, argv, "F:b:f:he6o:");
-        if (c == -1)
+        if (c == -1) {
             break;
+        }
         switch(c) {
         case 'h':
             help();
@@ -333,8 +334,9 @@ static int img_create(int argc, char **argv)
     }
 
     /* Get the filename */
-    if (optind >= argc)
+    if (optind >= argc) {
         help();
+    }
     filename = argv[optind++];
 
     if (options && !strcmp(options, "?")) {
@@ -473,8 +475,9 @@ static int img_check(int argc, char **argv)
     fmt = NULL;
     for(;;) {
         c = getopt(argc, argv, "f:h");
-        if (c == -1)
+        if (c == -1) {
             break;
+        }
         switch(c) {
         case 'h':
             help();
@@ -484,8 +487,9 @@ static int img_check(int argc, char **argv)
             break;
         }
     }
-    if (optind >= argc)
+    if (optind >= argc) {
         help();
+    }
     filename = argv[optind++];
 
     bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS);
@@ -549,8 +553,9 @@ static int img_commit(int argc, char **argv)
     fmt = NULL;
     for(;;) {
         c = getopt(argc, argv, "f:h");
-        if (c == -1)
+        if (c == -1) {
             break;
+        }
         switch(c) {
         case 'h':
             help();
@@ -560,8 +565,9 @@ static int img_commit(int argc, char **argv)
             break;
         }
     }
-    if (optind >= argc)
+    if (optind >= argc) {
         help();
+    }
     filename = argv[optind++];
 
     bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR);
@@ -685,8 +691,9 @@ static int img_convert(int argc, char **argv)
     flags = 0;
     for(;;) {
         c = getopt(argc, argv, "f:O:B:s:hce6o:");
-        if (c == -1)
+        if (c == -1) {
             break;
+        }
         switch(c) {
         case 'h':
             help();
@@ -719,7 +726,9 @@ static int img_convert(int argc, char **argv)
     }
 
     bs_n = argc - optind - 1;
-    if (bs_n < 1) help();
+    if (bs_n < 1) {
+        help();
+    }
 
     out_filename = argv[argc - 1];
 
@@ -907,8 +916,9 @@ static int img_convert(int argc, char **argv)
             }
             assert (remainder == 0);
 
-            if (n < cluster_sectors)
+            if (n < cluster_sectors) {
                 memset(buf + n * 512, 0, cluster_size - n * 512);
+            }
             if (is_not_zero(buf, cluster_size)) {
                 ret = bdrv_write_compressed(out_bs, sector_num, buf,
                                             cluster_sectors);
@@ -928,12 +938,14 @@ static int img_convert(int argc, char **argv)
         sector_num = 0; // total number of sectors converted so far
         for(;;) {
             nb_sectors = total_sectors - sector_num;
-            if (nb_sectors <= 0)
+            if (nb_sectors <= 0) {
                 break;
-            if (nb_sectors >= (IO_BUF_SIZE / 512))
+            }
+            if (nb_sectors >= (IO_BUF_SIZE / 512)) {
                 n = (IO_BUF_SIZE / 512);
-            else
+            } else {
                 n = nb_sectors;
+            }
 
             while (sector_num - bs_offset >= bs_sectors) {
                 bs_i ++;
@@ -945,8 +957,9 @@ static int img_convert(int argc, char **argv)
                    sector_num, bs_i, bs_offset, bs_sectors); */
             }
 
-            if (n > bs_offset + bs_sectors - sector_num)
+            if (n > bs_offset + bs_sectors - sector_num) {
                 n = bs_offset + bs_sectors - sector_num;
+            }
 
             if (has_zero_init) {
                 /* If the output image is being created as a copy on write image,
@@ -1082,8 +1095,9 @@ static int img_info(int argc, char **argv)
     fmt = NULL;
     for(;;) {
         c = getopt(argc, argv, "f:h");
-        if (c == -1)
+        if (c == -1) {
             break;
+        }
         switch(c) {
         case 'h':
             help();
@@ -1093,8 +1107,9 @@ static int img_info(int argc, char **argv)
             break;
         }
     }
-    if (optind >= argc)
+    if (optind >= argc) {
         help();
+    }
     filename = argv[optind++];
 
     bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING);
@@ -1105,11 +1120,12 @@ static int img_info(int argc, char **argv)
     bdrv_get_geometry(bs, &total_sectors);
     get_human_readable_size(size_buf, sizeof(size_buf), total_sectors * 512);
     allocated_size = get_allocated_file_size(filename);
-    if (allocated_size < 0)
+    if (allocated_size < 0) {
         snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
-    else
+    } else {
         get_human_readable_size(dsize_buf, sizeof(dsize_buf),
                                 allocated_size);
+    }
     printf("image: %s\n"
            "file format: %s\n"
            "virtual size: %s (%" PRId64 " bytes)\n"
@@ -1117,11 +1133,13 @@ static int img_info(int argc, char **argv)
            filename, fmt_name, size_buf,
            (total_sectors * 512),
            dsize_buf);
-    if (bdrv_is_encrypted(bs))
+    if (bdrv_is_encrypted(bs)) {
         printf("encrypted: yes\n");
+    }
     if (bdrv_get_info(bs, &bdi) >= 0) {
-        if (bdi.cluster_size != 0)
+        if (bdi.cluster_size != 0) {
             printf("cluster_size: %d\n", bdi.cluster_size);
+        }
     }
     bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename));
     if (backing_filename[0] != '\0') {
@@ -1154,8 +1172,9 @@ static int img_snapshot(int argc, char **argv)
     /* Parse commandline parameters */
     for(;;) {
         c = getopt(argc, argv, "la:c:d:h");
-        if (c == -1)
+        if (c == -1) {
             break;
+        }
         switch(c) {
         case 'h':
             help();
@@ -1195,8 +1214,9 @@ static int img_snapshot(int argc, char **argv)
         }
     }
 
-    if (optind >= argc)
+    if (optind >= argc) {
         help();
+    }
     filename = argv[optind++];
 
     /* Open the image */
@@ -1220,23 +1240,26 @@ static int img_snapshot(int argc, char **argv)
         sn.date_nsec = tv.tv_usec * 1000;
 
         ret = bdrv_snapshot_create(bs, &sn);
-        if (ret)
+        if (ret) {
             error("Could not create snapshot '%s': %d (%s)",
                 snapshot_name, ret, strerror(-ret));
+        }
         break;
 
     case SNAPSHOT_APPLY:
         ret = bdrv_snapshot_goto(bs, snapshot_name);
-        if (ret)
+        if (ret) {
             error("Could not apply snapshot '%s': %d (%s)",
                 snapshot_name, ret, strerror(-ret));
+        }
         break;
 
     case SNAPSHOT_DELETE:
         ret = bdrv_snapshot_delete(bs, snapshot_name);
-        if (ret)
+        if (ret) {
             error("Could not delete snapshot '%s': %d (%s)",
                 snapshot_name, ret, strerror(-ret));
+        }
         break;
     }
 
@@ -1264,8 +1287,9 @@ static int img_rebase(int argc, char **argv)
 
     for(;;) {
         c = getopt(argc, argv, "uhf:F:b:");
-        if (c == -1)
+        if (c == -1) {
             break;
+        }
         switch(c) {
         case 'h':
             help();
@@ -1285,8 +1309,9 @@ static int img_rebase(int argc, char **argv)
         }
     }
 
-    if ((optind >= argc) || !out_baseimg)
+    if ((optind >= argc) || !out_baseimg) {
         help();
+    }
     filename = argv[optind++];
 
     /*
-- 
1.7.3.2

[Qemu-devel] Re: [PATCH 6/7] Fix formatting and missing braces in qemu-img.c

[Stefan Hajnoczi]

On Mon, Dec 06, 2010 at 03:25:39PM +0100, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  qemu-img.c |   77 +++++++++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 51 insertions(+), 26 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] [PATCH 7/7] Fail if detecting an unknown option

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

This patch changes qemu-img to exit if an unknown option is detected,
instead of trying to continue with a set of arguments which may be
incorrect.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qemu-img.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index d812db0..f021a06 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -309,6 +309,7 @@ static int img_create(int argc, char **argv)
             break;
         }
         switch(c) {
+        case '?':
         case 'h':
             help();
             break;
@@ -479,6 +480,7 @@ static int img_check(int argc, char **argv)
             break;
         }
         switch(c) {
+        case '?':
         case 'h':
             help();
             break;
@@ -557,6 +559,7 @@ static int img_commit(int argc, char **argv)
             break;
         }
         switch(c) {
+        case '?':
         case 'h':
             help();
             break;
@@ -695,6 +698,7 @@ static int img_convert(int argc, char **argv)
             break;
         }
         switch(c) {
+        case '?':
         case 'h':
             help();
             break;
@@ -1099,6 +1103,7 @@ static int img_info(int argc, char **argv)
             break;
         }
         switch(c) {
+        case '?':
         case 'h':
             help();
             break;
@@ -1176,6 +1181,7 @@ static int img_snapshot(int argc, char **argv)
             break;
         }
         switch(c) {
+        case '?':
         case 'h':
             help();
             return 0;
@@ -1291,6 +1297,7 @@ static int img_rebase(int argc, char **argv)
             break;
         }
         switch(c) {
+        case '?':
         case 'h':
             help();
             return 0;
@@ -1505,6 +1512,7 @@ static int img_resize(int argc, char **argv)
             break;
         }
         switch(c) {
+        case '?':
         case 'h':
             help();
             break;
-- 
1.7.3.2

[Qemu-devel] Re: [PATCH 7/7] Fail if detecting an unknown option

[Stefan Hajnoczi]

On Mon, Dec 06, 2010 at 03:25:40PM +0100, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> This patch changes qemu-img to exit if an unknown option is detected,
> instead of trying to continue with a set of arguments which may be
> incorrect.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  qemu-img.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] Re: [PATCH v3 0/7] Cleanup qemu-img code

[Kevin Wolf]

Am 06.12.2010 15:25, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Hi,
> 
> These patches applies a number of cleanups to qemu-img.c as well as a
> minor bug in qemu-malloc.c. 
> 
> The handling of block help printing is moved to shared code, which
> allows the "?" detection to happen early in the parsing, instead of
> half way down img_create() and img_convert(). I would like to see this
> happen as I would like to pull some of the code out of img_create()
> and into block.c so it can be shared with qemu and qemu-img.
> 
> In addition there is a couple of patches to clean up the error
> handling in qemu-img.c and make it more consistent.
> 
> The formatting patch is solely because the last patch wanted to
> change code next to the badly formatted code, and I didn't want to
> pollute the patch with the formatting fixed.
> 
> The seventh patch fixes qemu-img to exit on detection of unknown
> options instead of continuing with a potentially wrong set of
> arguments.
> 
> v3 applies a number of changes discussed on irc and email. This is the
> grow to seven from three patches series.
> 
> Cheers,
> Jes
> 
> Jes Sorensen (7):
>   Add missing tracing to qemu_mallocz()
>   Use qemu_mallocz() instead of calloc() in img_convert()
>   img_convert(): Only try to free bs[] entries if bs is valid.
>   Make error handling more consistent in img_create() and img_resize()
>   Consolidate printing of block driver options
>   Fix formatting and missing braces in qemu-img.c
>   Fail if detecting an unknown option
> 
>  qemu-img.c    |  162 +++++++++++++++++++++++++++++++++++++++-----------------
>  qemu-malloc.c |    5 ++-
>  2 files changed, 117 insertions(+), 50 deletions(-)

Thanks, applied all except patch 4, which breaks the build. Please
resend a new version of patch 4 as a single patch without the rest of
the series.

Kevin

[Qemu-devel] [PATCH 4/7] Make error handling more consistent in img_create() and img_resize()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 qemu-img.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index aded72d..2deac67 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -314,13 +314,15 @@ static int img_create(int argc, char **argv)
     drv = bdrv_find_format(fmt);
     if (!drv) {
         error("Unknown file format '%s'", fmt);
-        return 1;
+        ret = -1;
+        goto out;
     }
 
     proto_drv = bdrv_find_protocol(filename);
     if (!proto_drv) {
         error("Unknown protocol '%s'", filename);
-        return 1;
+        ret = -1;
+        goto out;
     }
 
     create_options = append_option_parameters(create_options,
@@ -1432,7 +1434,7 @@ static int img_resize(int argc, char **argv)
     int c, ret, relative;
     const char *filename, *fmt, *size;
     int64_t n, total_size;
-    BlockDriverState *bs;
+    BlockDriverState *bs = NULL;
     QEMUOptionParameter *param;
     QEMUOptionParameter resize_options[] = {
         {
@@ -1483,14 +1485,16 @@ static int img_resize(int argc, char **argv)
     param = parse_option_parameters("", resize_options, NULL);
     if (set_option_parameter(param, BLOCK_OPT_SIZE, size)) {
         /* Error message already printed when size parsing fails */
-        exit(1);
+        ret = -1;
+        goto out;
     }
     n = get_option_parameter(param, BLOCK_OPT_SIZE)->value.n;
     free_option_parameters(param);
 
     bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR);
     if (!bs) {
-        return 1;
+        ret = -1;
+        goto out;
     }
 
     if (relative) {
-- 
1.7.3.2

[Qemu-devel] Re: [PATCH 4/7] Make error handling more consistent in img_create() and img_resize()

[Kevin Wolf]

Am 06.12.2010 16:45, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  qemu-img.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)

> @@ -1432,7 +1434,7 @@ static int img_resize(int argc, char **argv)
>      int c, ret, relative;
>      const char *filename, *fmt, *size;
>      int64_t n, total_size;
> -    BlockDriverState *bs;
> +    BlockDriverState *bs = NULL;
>      QEMUOptionParameter *param;
>      QEMUOptionParameter resize_options[] = {
>          {
> @@ -1483,14 +1485,16 @@ static int img_resize(int argc, char **argv)
>      param = parse_option_parameters("", resize_options, NULL);
>      if (set_option_parameter(param, BLOCK_OPT_SIZE, size)) {
>          /* Error message already printed when size parsing fails */
> -        exit(1);
> +        ret = -1;
> +        goto out;
>      }
>      n = get_option_parameter(param, BLOCK_OPT_SIZE)->value.n;
>      free_option_parameters(param);
>  
>      bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR);
>      if (!bs) {
> -        return 1;
> +        ret = -1;
> +        goto out;
>      }
>  
>      if (relative) {

bdrv_delete doesn't check for NULL, so this still isn't enough. Try
something like "qemu-img resize -f vmdx foo +0" and you'll get a segfault.

Kevin

[Qemu-devel] Re: [PATCH 4/7] Make error handling more consistent in img_create() and img_resize()

[Jes Sorensen]

On 12/06/10 16:57, Kevin Wolf wrote:
> bdrv_delete doesn't check for NULL, so this still isn't enough. Try
> something like "qemu-img resize -f vmdx foo +0" and you'll get a segfault.

Grrrr :(

It's a bummer things are so inconsistent throughout QEMU, most of the
free() functions can handle it.

Updated patch in a minute - sorry.

Thanks,
Jes