diff mbox

[v4,1/3] vl: Add a flags to define parameters with optional arguments.

Message ID 1441395005-14907-2-git-send-email-lvivier@redhat.com
State New
Headers show

Commit Message

Laurent Vivier Sept. 4, 2015, 7:30 p.m. UTC
The goal is to be able to use '-help' alone, or with
a sub-section, i.e. '-help network,usb'.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 vl.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

Comments

Eric Blake Sept. 4, 2015, 7:42 p.m. UTC | #1
On 09/04/2015 01:30 PM, Laurent Vivier wrote:
> The goal is to be able to use '-help' alone, or with
> a sub-section, i.e. '-help network,usb'.

Uggh. I hate reinventing the wheel.  We aren't using getopt_long_only();
but if we were, the ONLY way to specify optional arguments to a long
option is by using the = sign, and not by space separation.

That is, '-help=network,usb' would work, but '-help network,usb' would
NOT be seen as arguments to -help.

While I'd really rather we just use getopt_long_only(), that's a much
bigger change.  But we should at least mirror its semantics, because it
is VERY confusing to have subtly different command-line behavior than
most apps out there.

> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  vl.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
Eric Blake Sept. 4, 2015, 7:52 p.m. UTC | #2
On 09/04/2015 01:42 PM, Eric Blake wrote:
> On 09/04/2015 01:30 PM, Laurent Vivier wrote:
>> The goal is to be able to use '-help' alone, or with
>> a sub-section, i.e. '-help network,usb'.
> 
> Uggh. I hate reinventing the wheel.  We aren't using getopt_long_only();
> but if we were, the ONLY way to specify optional arguments to a long
> option is by using the = sign, and not by space separation.

Also, we tend to avoid trailing '.' in subject lines.
Stefan Weil Sept. 4, 2015, 7:55 p.m. UTC | #3
Am 04.09.2015 um 21:42 schrieb Eric Blake:
> On 09/04/2015 01:30 PM, Laurent Vivier wrote:
>> The goal is to be able to use '-help' alone, or with
>> a sub-section, i.e. '-help network,usb'.
> Uggh. I hate reinventing the wheel.  We aren't using getopt_long_only();
> but if we were, the ONLY way to specify optional arguments to a long
> option is by using the = sign, and not by space separation.
>
> That is, '-help=network,usb' would work, but '-help network,usb' would
> NOT be seen as arguments to -help.
>
> While I'd really rather we just use getopt_long_only(), that's a much
> bigger change.  But we should at least mirror its semantics, because it
> is VERY confusing to have subtly different command-line behavior than
> most apps out there.

Wasn't there a plan for a version jump (QEMU v3) with the next release?

Then we could really go a step towards getopt_long_only
and require options written like --help instead of -help.
Today both variants work, so we would have to remove
support for the -help form.

Stefan
Eric Blake Sept. 4, 2015, 8:06 p.m. UTC | #4
On 09/04/2015 01:55 PM, Stefan Weil wrote:
> Am 04.09.2015 um 21:42 schrieb Eric Blake:
>> On 09/04/2015 01:30 PM, Laurent Vivier wrote:
>>> The goal is to be able to use '-help' alone, or with
>>> a sub-section, i.e. '-help network,usb'.
>> Uggh. I hate reinventing the wheel.  We aren't using getopt_long_only();
>> but if we were, the ONLY way to specify optional arguments to a long
>> option is by using the = sign, and not by space separation.
>>
>> That is, '-help=network,usb' would work, but '-help network,usb' would
>> NOT be seen as arguments to -help.
>>
>> While I'd really rather we just use getopt_long_only(), that's a much
>> bigger change.  But we should at least mirror its semantics, because it
>> is VERY confusing to have subtly different command-line behavior than
>> most apps out there.
> 
> Wasn't there a plan for a version jump (QEMU v3) with the next release?

I don't know; but feature-wise, if MTTCG and introspection both land in
the next release, that would be major enough in my book to be worth
naming things 3.0 if people like it.

> 
> Then we could really go a step towards getopt_long_only
> and require options written like --help instead of -help.

getopt_long_only() is an awkward interface. Most apps should use
getopt_long() instead.  What getopt_long_only() does is allow you to use
'-long' or any unambiguous prefix instead of '--long', provided that
'-l' is not a short option.  Note that we have '-h' as an alias for
long-option help, we'd run into fixable issues (since getopt_long_only()
allows unambiguous abbreviations, getting rid of short -h would allow
'-h' to become unambiguous for long option '--help', as long as no other
long option starts with h [oops, we have -hda]; on top of that, you can
also explicitly list '--h' as a long option, which would then make it
take preference over any other substring of a longer name).  So it would
still be possible to switch to getopt_long_only() while still keeping
support for '-help' to work.

Meanwhile, where we'd run into trouble is the fact that '-m' is
different than '-machine'; if we use getopt_long(), then '-machine'
would break.

> Today both variants work, so we would have to remove
> support for the -help form.

Well, even if we don't remove support for '-help', we could at least
update our documentation to mention that '--long' forms already work,
and that they are the favored spelling (as we may want to eventually get
rid of -short forms down the road).  But I agree that gratuitously
breaking existing -short form command lines without ample warning time
is not the right approach to take, and don't know if we have given
enough warning, regardless of whether the next release is named 2.5 or 3.0.
diff mbox

Patch

diff --git a/vl.c b/vl.c
index 584ca88..a4fa7e5 100644
--- a/vl.c
+++ b/vl.c
@@ -1932,6 +1932,7 @@  static void help(int exitcode)
 }
 
 #define HAS_ARG 0x0001
+#define IS_OPT  0x0002
 
 typedef struct QEMUOption {
     const char *name;
@@ -2703,12 +2704,21 @@  static const QEMUOption *lookup_opt(int argc, char **argv,
         popt++;
     }
     if (popt->flags & HAS_ARG) {
-        if (optind >= argc) {
-            error_report("requires an argument");
-            exit(1);
+        if (popt->flags & IS_OPT) {
+            if (optind < argc && argv[optind][0] != '-') {
+                optarg = argv[optind++];
+                loc_set_cmdline(argv, optind - 2, 2);
+            } else {
+                optarg = NULL;
+            }
+        } else {
+            if (optind >= argc) {
+                    error_report("requires an argument");
+                    exit(1);
+            }
+            optarg = argv[optind++];
+            loc_set_cmdline(argv, optind - 2, 2);
         }
-        optarg = argv[optind++];
-        loc_set_cmdline(argv, optind - 2, 2);
     } else {
         optarg = NULL;
     }