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