[Qemu-devel] [PATCH v2 0/3] Cleanup qemu-img code

[Qemu-devel] [PATCH v2 0/3] Cleanup qemu-img code

[Jes.Sorensen]

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

Hi,

These patches moves the handling of block help printing 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.

The formatting patch is solely because the third 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 third patch fixes qemu-img to exit on detection of unknown options
instead of continuing with a potentially wrong set of arguments.

New in v2: Add missing free_option_parameters() and handle the help()
case in the general switch() statements for the getopt() output.

Cheers,
Jes

Jes Sorensen (3):
  Consolidate printing of block driver options
  Fix formatting and missing braces in qemu-img.c
  Fail if detecting an unknown option

 qemu-img.c |  132 ++++++++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 97 insertions(+), 35 deletions(-)

-- 
1.7.3.2

[Qemu-devel] [PATCH 1/3] 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 |   47 ++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index fa77ac0..7863835 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) {
@@ -328,11 +360,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);
@@ -694,6 +721,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 out2;
+    }
+
     if (bs_n > 1 && out_baseimg) {
         error("-B makes no sense when concatenating multiple input images");
         return 1;
@@ -749,10 +781,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);
@@ -984,6 +1012,7 @@ out:
         }
     }
     free(bs);
+out2:
     if (ret) {
         return 1;
     }
-- 
1.7.3.2

Re: [Qemu-devel] [PATCH 1/3] Consolidate printing of block driver options

[Stefan Hajnoczi]

On Mon, Dec 6, 2010 at 8:17 AM,  <Jes.Sorensen@redhat.com> wrote:
> @@ -694,6 +721,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 out2;
> +    }
> +
>     if (bs_n > 1 && out_baseimg) {
>         error("-B makes no sense when concatenating multiple input images");
>         return 1;

Why goto out2 and not just return like the bs > 1 && out_baseimg check?

Stefan

Re: [Qemu-devel] [PATCH 1/3] Consolidate printing of block driver options

[Jes Sorensen]

On 12/06/10 10:32, Stefan Hajnoczi wrote:
> On Mon, Dec 6, 2010 at 8:17 AM,  <Jes.Sorensen@redhat.com> wrote:
>> @@ -694,6 +721,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 out2;
>> +    }
>> +
>>     if (bs_n > 1 && out_baseimg) {
>>         error("-B makes no sense when concatenating multiple input images");
>>         return 1;
> 
> Why goto out2 and not just return like the bs > 1 && out_baseimg check?

It is cleaner, I'd rather convert the bs_n test to do it too.

Cheers,
Jes

Re: [Qemu-devel] [PATCH 1/3] Consolidate printing of block driver options

[Stefan Hajnoczi]

On Mon, Dec 6, 2010 at 10:20 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> On 12/06/10 10:32, Stefan Hajnoczi wrote:
>> On Mon, Dec 6, 2010 at 8:17 AM,  <Jes.Sorensen@redhat.com> wrote:
>>> @@ -694,6 +721,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 out2;
>>> +    }
>>> +
>>>     if (bs_n > 1 && out_baseimg) {
>>>         error("-B makes no sense when concatenating multiple input images");
>>>         return 1;
>>
>> Why goto out2 and not just return like the bs > 1 && out_baseimg check?
>
> It is cleaner, I'd rather convert the bs_n test to do it too.

"out2" tells me nothing and is just indirection for a return.  At this
point no resources have been acquired and it is simplest to bail out
early.

The segfault on out is fixed by setting up bs_n and bs[] together
instead of doing checks in between (that way we never have bs_n > 0
and bs == NULL).  Or by adding a check for bs != NULL to the out
cleanup code.  Then it would be safe to use out.

Stefan

Re: [Qemu-devel] [PATCH 1/3] Consolidate printing of block driver options

[Jes Sorensen]

On 12/06/10 11:37, Stefan Hajnoczi wrote:
> On Mon, Dec 6, 2010 at 10:20 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
>> On 12/06/10 10:32, Stefan Hajnoczi wrote:
>>> On Mon, Dec 6, 2010 at 8:17 AM,  <Jes.Sorensen@redhat.com> wrote:
>>> Why goto out2 and not just return like the bs > 1 && out_baseimg check?
>>
>> It is cleaner, I'd rather convert the bs_n test to do it too.
> 
> "out2" tells me nothing and is just indirection for a return.  At this
> point no resources have been acquired and it is simplest to bail out
> early.

A consistent out path is more likely to catch issues if the code is
modified later. I am not a big fan of the random mix of return vs goto
out that we spray over the code.... or having help() call exit() for
that matter.

Cheers,
Jes

Re: [Qemu-devel] [PATCH 1/3] Consolidate printing of block driver options

[Stefan Hajnoczi]

On Mon, Dec 6, 2010 at 10:47 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> On 12/06/10 11:37, Stefan Hajnoczi wrote:
>> On Mon, Dec 6, 2010 at 10:20 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
>>> On 12/06/10 10:32, Stefan Hajnoczi wrote:
>>>> On Mon, Dec 6, 2010 at 8:17 AM,  <Jes.Sorensen@redhat.com> wrote:
>>>> Why goto out2 and not just return like the bs > 1 && out_baseimg check?
>>>
>>> It is cleaner, I'd rather convert the bs_n test to do it too.
>>
>> "out2" tells me nothing and is just indirection for a return.  At this
>> point no resources have been acquired and it is simplest to bail out
>> early.
>
> A consistent out path is more likely to catch issues if the code is
> modified later. I am not a big fan of the random mix of return vs goto
> out that we spray over the code.... or having help() call exit() for
> that matter.

img_convert() wasn't random before your patch: return statements
before the first resource allocation, gotos afterwards. :P

I see what you're saying though.  How about making out work in all
cases and consistently using goto out?

Stefan

[Qemu-devel] [PATCH 2/3] 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 7863835..2a54ae2 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, "?")) {
@@ -471,8 +473,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();
@@ -482,8 +485,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);
@@ -547,8 +551,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();
@@ -558,8 +563,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);
@@ -683,8 +689,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();
@@ -717,7 +724,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];
 
@@ -908,8 +917,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);
@@ -929,12 +939,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 ++;
@@ -946,8 +958,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] [PATCH 3/3] 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 2a54ae2..3e3ca36 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;
@@ -477,6 +478,7 @@ static int img_check(int argc, char **argv)
             break;
         }
         switch(c) {
+        case '?':
         case 'h':
             help();
             break;
@@ -555,6 +557,7 @@ static int img_commit(int argc, char **argv)
             break;
         }
         switch(c) {
+        case '?':
         case 'h':
             help();
             break;
@@ -693,6 +696,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

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

[Stefan Hajnoczi]

On Mon, Dec 6, 2010 at 8:17 AM,  <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>