Patchwork [3/3] Fail if detecting an unknown option

login
register
mail settings
Submitter Jes Sorensen
Date Dec. 2, 2010, 5:46 p.m.
Message ID <1291312009-24351-4-git-send-email-Jes.Sorensen@redhat.com>
Download mbox | patch
Permalink /patch/74007/
State New
Headers show

Comments

Jes Sorensen - Dec. 2, 2010, 5:46 p.m.
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(-)
Stefan Hajnoczi - Dec. 3, 2010, 11:46 a.m.
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
Kevin Wolf - Dec. 3, 2010, 12:30 p.m.
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
Jes Sorensen - Dec. 6, 2010, 8:01 a.m.
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
Jes Sorensen - Dec. 6, 2010, 8:02 a.m.
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
Kevin Wolf - Dec. 6, 2010, 9:37 a.m.
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
Jes Sorensen - Dec. 6, 2010, 10:20 a.m.
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

Patch

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;
         }