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

[Qemu-devel] [PATCH 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.

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 |  171 +++++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 136 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 |   46 +++++++++++++++++++++++++++++++++++++---------
 1 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index fa77ac0..99f30b3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -188,6 +188,32 @@ 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);
+    return 0;
+}
+
 static BlockDriverState *bdrv_new_open(const char *filename,
                                        const char *fmt,
                                        int flags)
@@ -310,6 +336,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 +359,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 +720,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 +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);
@@ -984,6 +1011,7 @@ out:
         }
     }
     free(bs);
+out2:
     if (ret) {
         return 1;
     }
-- 
1.7.3.2

[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>
---
 qemu-img.c |   77 +++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 99f30b3..d0dc445 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -304,8 +304,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();
@@ -332,8 +333,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, "?")) {
@@ -470,8 +472,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();
@@ -481,8 +484,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);
@@ -546,8 +550,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();
@@ -557,8 +562,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);
@@ -682,8 +688,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();
@@ -716,7 +723,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,
@@ -1081,8 +1094,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();
@@ -1092,8 +1106,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);
@@ -1104,11 +1119,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"
@@ -1116,11 +1132,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') {
@@ -1153,8 +1171,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();
@@ -1194,8 +1213,9 @@ static int img_snapshot(int argc, char **argv)
         }
     }
 
-    if (optind >= argc)
+    if (optind >= argc) {
         help();
+    }
     filename = argv[optind++];
 
     /* Open the image */
@@ -1219,23 +1239,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;
     }
 
@@ -1263,8 +1286,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();
@@ -1284,8 +1308,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 |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index d0dc445..f2e1c94 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -304,6 +304,12 @@ static int img_create(int argc, char **argv)
     flags = 0;
     for(;;) {
         c = getopt(argc, argv, "F:b:f:he6o:");
+        /*
+         * Fail if we detect an unknown argument
+         */
+        if (c == '?') {
+            return 1;
+        }
         if (c == -1) {
             break;
         }
@@ -472,6 +478,12 @@ static int img_check(int argc, char **argv)
     fmt = NULL;
     for(;;) {
         c = getopt(argc, argv, "f:h");
+        /*
+         * Fail if we detect an unknown argument
+         */
+        if (c == '?') {
+            return 1;
+        }
         if (c == -1) {
             break;
         }
@@ -550,6 +562,12 @@ static int img_commit(int argc, char **argv)
     fmt = NULL;
     for(;;) {
         c = getopt(argc, argv, "f:h");
+        /*
+         * Fail if we detect an unknown argument
+         */
+        if (c == '?') {
+            return 1;
+        }
         if (c == -1) {
             break;
         }
@@ -688,6 +706,12 @@ static int img_convert(int argc, char **argv)
     flags = 0;
     for(;;) {
         c = getopt(argc, argv, "f:O:B:s:hce6o:");
+        /*
+         * Fail if we detect an unknown argument
+         */
+        if (c == '?') {
+            return 1;
+        }
         if (c == -1) {
             break;
         }
@@ -1094,6 +1118,12 @@ static int img_info(int argc, char **argv)
     fmt = NULL;
     for(;;) {
         c = getopt(argc, argv, "f:h");
+        /*
+         * Fail if we detect an unknown argument
+         */
+        if (c == '?') {
+            return 1;
+        }
         if (c == -1) {
             break;
         }
@@ -1171,6 +1201,12 @@ static int img_snapshot(int argc, char **argv)
     /* Parse commandline parameters */
     for(;;) {
         c = getopt(argc, argv, "la:c:d:h");
+        /*
+         * Fail if we detect an unknown argument
+         */
+        if (c == '?') {
+            return 1;
+        }
         if (c == -1) {
             break;
         }
@@ -1286,6 +1322,12 @@ static int img_rebase(int argc, char **argv)
 
     for(;;) {
         c = getopt(argc, argv, "uhf:F:b:");
+        /*
+         * Fail if we detect an unknown argument
+         */
+        if (c == '?') {
+            return 1;
+        }
         if (c == -1) {
             break;
         }
@@ -1500,6 +1542,12 @@ static int img_resize(int argc, char **argv)
     fmt = NULL;
     for(;;) {
         c = getopt(argc, argv, "f:h");
+        /*
+         * Fail if we detect an unknown argument
+         */
+        if (c == '?') {
+            return 1;
+        }
         if (c == -1) {
             break;
         }
-- 
1.7.3.2

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

[Stefan Hajnoczi]

On Thu, Dec 2, 2010 at 5:46 PM,  <Jes.Sorensen@redhat.com> wrote:
> @@ -188,6 +188,32 @@ 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);

> @@ -694,6 +720,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 +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);
> @@ -984,6 +1011,7 @@ out:
>         }
>     }
>     free(bs);
> +out2:

Not needed, out is fine.  All those free functions are nops on NULL pointers.

Even simpler would be to just return early instead of jumping to a
label.  The other checks at the top of the function also do that.

Stefan

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

[Jes Sorensen]

On 12/03/10 11:57, Stefan Hajnoczi wrote:
> On Thu, Dec 2, 2010 at 5:46 PM,  <Jes.Sorensen@redhat.com> wrote:
>> +    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);

Hmmm good point

>> @@ -694,6 +720,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 +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);
>> @@ -984,6 +1011,7 @@ out:
>>         }
>>     }
>>     free(bs);
>> +out2:
> 
> Not needed, out is fine.  All those free functions are nops on NULL pointers.

Actually tried that, but it segfaulted, which is why I added out2.

Cheers,
Jes

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

[Stefan Hajnoczi]

On Fri, Dec 3, 2010 at 10:57 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Dec 2, 2010 at 5:46 PM,  <Jes.Sorensen@redhat.com> wrote:
>> @@ -694,6 +720,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 +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);
>> @@ -984,6 +1011,7 @@ out:
>>         }
>>     }
>>     free(bs);
>> +out2:
>
> Not needed, out is fine.  All those free functions are nops on NULL pointers.

Sorry, my comment is incorrect.  It is possible for bs_n > 0 and we'll
try to free bs[i].

Stefan

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

[Stefan Hajnoczi]

On Thu, Dec 2, 2010 at 5:46 PM,  <Jes.Sorensen@redhat.com> wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  qemu-img.c |   77 +++++++++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 51 insertions(+), 26 deletions(-)

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

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

[Stefan Hajnoczi]

On Thu, Dec 2, 2010 at 5:46 PM,  <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 |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 48 insertions(+), 0 deletions(-)

Do we get a silent exit if an unknown option is detected?  Normally
programs print their help/usage when this happens.

Stefan

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

[Kevin Wolf]

Am 02.12.2010 18:46, schrieb Jes.Sorensen@redhat.com:
> 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 |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 48 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index d0dc445..f2e1c94 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -304,6 +304,12 @@ static int img_create(int argc, char **argv)
>      flags = 0;
>      for(;;) {
>          c = getopt(argc, argv, "F:b:f:he6o:");
> +        /*
> +         * Fail if we detect an unknown argument
> +         */
> +        if (c == '?') {
> +            return 1;
> +        }
>          if (c == -1) {
>              break;
>          }

Why not making it another case in the switch statement below instead of
an additional if?

Kevin

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

[Jes Sorensen]

On 12/03/10 12:46, Stefan Hajnoczi wrote:
> On Thu, Dec 2, 2010 at 5:46 PM,  <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 |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 48 insertions(+), 0 deletions(-)
> 
> Do we get a silent exit if an unknown option is detected?  Normally
> programs print their help/usage when this happens.
> 
> Stefan

Fixed in the next version :)

Cheers,
Jes

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

[Jes Sorensen]

On 12/03/10 13:30, Kevin Wolf wrote:
> Am 02.12.2010 18:46, schrieb Jes.Sorensen@redhat.com:
>> diff --git a/qemu-img.c b/qemu-img.c
>> index d0dc445..f2e1c94 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -304,6 +304,12 @@ static int img_create(int argc, char **argv)
>>      flags = 0;
>>      for(;;) {
>>          c = getopt(argc, argv, "F:b:f:he6o:");
>> +        /*
>> +         * Fail if we detect an unknown argument
>> +         */
>> +        if (c == '?') {
>> +            return 1;
>> +        }
>>          if (c == -1) {
>>              break;
>>          }
> 
> Why not making it another case in the switch statement below instead of
> an additional if?

There is a perfectly logical explanation for that. Doing that would
require for me to have clue, which is a bit much to expect :)

That said, we should really do the same for the c == -1 case as well.

Fixed in next version.

Cheers,
Jes

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

[Kevin Wolf]

Am 06.12.2010 09:02, schrieb Jes Sorensen:
> On 12/03/10 13:30, Kevin Wolf wrote:
>> Am 02.12.2010 18:46, schrieb Jes.Sorensen@redhat.com:
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index d0dc445..f2e1c94 100644
>>> --- a/qemu-img.c
>>> +++ b/qemu-img.c
>>> @@ -304,6 +304,12 @@ static int img_create(int argc, char **argv)
>>>      flags = 0;
>>>      for(;;) {
>>>          c = getopt(argc, argv, "F:b:f:he6o:");
>>> +        /*
>>> +         * Fail if we detect an unknown argument
>>> +         */
>>> +        if (c == '?') {
>>> +            return 1;
>>> +        }
>>>          if (c == -1) {
>>>              break;
>>>          }
>>
>> Why not making it another case in the switch statement below instead of
>> an additional if?
> 
> There is a perfectly logical explanation for that. Doing that would
> require for me to have clue, which is a bit much to expect :)
> 
> That said, we should really do the same for the c == -1 case as well.

That's what I thought at first, too. But then the break relates to the
switch instead of the for, so it would have to become a goto to a new
label. Probably not a big improvement...

Kevin

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

[Jes Sorensen]

On 12/06/10 10:37, Kevin Wolf wrote:
> Am 06.12.2010 09:02, schrieb Jes Sorensen:
>> On 12/03/10 13:30, Kevin Wolf wrote:
>> There is a perfectly logical explanation for that. Doing that would
>> require for me to have clue, which is a bit much to expect :)
>>
>> That said, we should really do the same for the c == -1 case as well.
> 
> That's what I thought at first, too. But then the break relates to the
> switch instead of the for, so it would have to become a goto to a new
> label. Probably not a big improvement...

Yeah, it hit me the moment I hit send, so ignore that comment.

Cheers,
Jes

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>

[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