ovs-vsctl: Allow command-specific options to mingle with global options.

Until now, a command like "ovs-vsctl --may-exist add-br br0" yielded a
confusing error message.  Users had to realize that the correct form was
"ovs-vsctl -- --may-exist add-br br0", but instead they often reported a
bug or gave up in frustration.  Even though the behavior was documented, it
was counterintuitive.

This commit allows command-specific options to be mixed with global
options, making both forms of the command listed above equally acceptable.

CC: 691508@bugs.debian.org
Reported-by: Adam Heath <doogie@brainfood.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Kyle Mestery <kmestery@cisco.com>

AUTHORS

@@ -83,6 +83,7 @@ The following additional people are mentioned in commit logs as having
 provided helpful bug reports or suggestions.
 
 Aaron M. Ucko           ucko@debian.org
+Adam Heath              doogie@brainfood.com
 Ahmed Bilal             numan252@gmail.com
 Alan Shieh              ashieh@nicira.com
 Alban Browaeys          prahal@yahoo.com

tests/ovs-vsctl.at

@@ -15,7 +15,7 @@ dnl RUN_OVS_VSCTL(COMMAND, ...)
 dnl
 dnl Executes each ovs-vsctl COMMAND.
 m4_define([RUN_OVS_VSCTL],
-  [m4_foreach([command], [$@], [ovs-vsctl --timeout=5 --no-wait -vreconnect:emer --db=unix:socket -- command
+  [m4_foreach([command], [$@], [ovs-vsctl --timeout=5 --no-wait -vreconnect:emer --db=unix:socket command
 ])])
 m4_define([RUN_OVS_VSCTL_ONELINE],
   [m4_foreach([command], [$@], [ovs-vsctl --timeout=5 --no-wait -vreconnect:emer --db=unix:socket --oneline -- command
@@ -672,6 +672,17 @@ AT_CLEANUP
 AT_SETUP([database commands -- negative checks])
 AT_KEYWORDS([ovs-vsctl])
 OVS_VSCTL_SETUP
+
+AT_CHECK([ovs-vsctl --may-exist],
+  [1], [ignore], [ovs-vsctl: missing command name (use --help for help)
+], [OVS_VSCTL_CLEANUP])
+AT_CHECK([ovs-vsctl --may-exist --],
+  [1], [ignore], [ovs-vsctl: missing command name (use --help for help)
+], [OVS_VSCTL_CLEANUP])
+AT_CHECK([ovs-vsctl -- --may-exist],
+  [1], [ignore], [ovs-vsctl: missing command name (use --help for help)
+], [OVS_VSCTL_CLEANUP])
+
 AT_CHECK([RUN_OVS_VSCTL([add-br br0])],
   [0], [ignore], [], [OVS_VSCTL_CLEANUP])
 AT_CHECK([RUN_OVS_VSCTL([add-br br1])], 

utilities/ovs-vsctl.8.in

@@ -43,9 +43,9 @@ implemented as a single atomic transaction against the database.
 The \fBovs\-vsctl\fR command line begins with global options (see
 \fBOPTIONS\fR below for details).  The global options are followed by
 one or more commands.  Each command should begin with \fB\-\-\fR by
-itself as a command-line argument, to separate it from the global
+itself as a command-line argument, to separate it from the following
-options and following commands.  (If the first command does not have
+commands.  (The \fB\-\-\fR before the first command is optional.)  The
-any options, then the first \fB\-\-\fR may be omitted.)  The command
+command
 itself starts with command-specific options, if any, followed by the
 command name and any arguments.  See \fBEXAMPLES\fR below for syntax
 examples.
@@ -770,10 +770,9 @@ Delete bridge \fBbr0\fR, reporting an error if it does not exist:
 .IP
 .B "ovs\-vsctl del\-br br0"
 .PP
-Delete bridge \fBbr0\fR if it exists (the \fB\-\-\fR is required to
+Delete bridge \fBbr0\fR if it exists:
-separate \fBdel\-br\fR's options from the global options):
 .IP
-.B "ovs\-vsctl \-\- \-\-if\-exists del\-br br0"
+.B "ovs\-vsctl \-\-if\-exists del\-br br0"
 .PP
 Set the \fBqos\fR column of the \fBPort\fR record for \fBeth0\fR to
 point to a new \fBQoS\fR record, which in turn points with its queue 0

utilities/ovs-vsctl.c

@@ -131,12 +131,14 @@ static void vsctl_exit(int status) NO_RETURN;
 static void vsctl_fatal(const char *, ...) PRINTF_FORMAT(1, 2) NO_RETURN;
 static char *default_db(void);
 static void usage(void) NO_RETURN;
-static void parse_options(int argc, char *argv[]);
+static void parse_options(int argc, char *argv[], struct shash *local_options);
 static bool might_write_to_db(char **argv);
 
 static struct vsctl_command *parse_commands(int argc, char *argv[],
+                                            struct shash *local_options,
                                             size_t *n_commandsp);
-static void parse_command(int argc, char *argv[], struct vsctl_command *);
+static void parse_command(int argc, char *argv[], struct shash *local_options,
+                          struct vsctl_command *);
 static const struct vsctl_command_syntax *find_command(const char *name);
 static void run_prerequisites(struct vsctl_command[], size_t n_commands,
                               struct ovsdb_idl *);
@@ -159,6 +161,7 @@ main(int argc, char *argv[])
     extern struct vlog_module VLM_reconnect;
     struct ovsdb_idl *idl;
     struct vsctl_command *commands;
+    struct shash local_options;
     unsigned int seqno;
     size_t n_commands;
     char *args;
@@ -174,8 +177,10 @@ main(int argc, char *argv[])
     VLOG(might_write_to_db(argv) ? VLL_INFO : VLL_DBG, "Called as %s", args);
 
     /* Parse command line. */
-    parse_options(argc, argv);
+    shash_init(&local_options);
-    commands = parse_commands(argc - optind, argv + optind, &n_commands);
+    parse_options(argc, argv, &local_options);
+    commands = parse_commands(argc - optind, argv + optind, &local_options,
+                              &n_commands);
 
     if (timeout) {
         time_alarm(timeout);
@@ -208,8 +213,32 @@ main(int argc, char *argv[])
     }
 }
 
+static struct option *
+find_option(const char *name, struct option *options, size_t n_options)
+{
+    size_t i;
+
+    for (i = 0; i < n_options; i++) {
+        if (!strcmp(options[i].name, name)) {
+            return &options[i];
+        }
+    }
+    return NULL;
+}
+
+static struct option *
+add_option(struct option **optionsp, size_t *n_optionsp,
+           size_t *allocated_optionsp)
+{
+    if (*n_optionsp >= *allocated_optionsp) {
+        *optionsp = x2nrealloc(*optionsp, allocated_optionsp,
+                               sizeof **optionsp);
+    }
+    return &(*optionsp)[(*n_optionsp)++];
+}
+
 static void
-parse_options(int argc, char *argv[])
+parse_options(int argc, char *argv[], struct shash *local_options)
 {
     enum {
         OPT_DB = UCHAR_MAX + 1,
@@ -218,10 +247,11 @@ parse_options(int argc, char *argv[])
         OPT_NO_WAIT,
         OPT_DRY_RUN,
         OPT_PEER_CA_CERT,
+        OPT_LOCAL,
         VLOG_OPTION_ENUMS,
         TABLE_OPTION_ENUMS
     };
-    static struct option long_options[] = {
+    static const struct option global_long_options[] = {
         {"db", required_argument, NULL, OPT_DB},
         {"no-syslog", no_argument, NULL, OPT_NO_SYSLOG},
         {"no-wait", no_argument, NULL, OPT_NO_WAIT},
@@ -236,18 +266,75 @@ parse_options(int argc, char *argv[])
         {"peer-ca-cert", required_argument, NULL, OPT_PEER_CA_CERT},
         {NULL, 0, NULL, 0},
     };
+    const int n_global_long_options = ARRAY_SIZE(global_long_options) - 1;
     char *tmp, *short_options;
 
-    tmp = long_options_to_short_options(long_options);
+    const struct vsctl_command_syntax *p;
+    struct option *options, *o;
+    size_t allocated_options;
+    size_t n_options;
+    size_t i;
+
+    tmp = long_options_to_short_options(global_long_options);
     short_options = xasprintf("+%s", tmp);
     free(tmp);
 
+    /* We want to parse both global and command-specific options here, but
+     * getopt_long() isn't too convenient for the job.  We copy our global
+     * options into a dynamic array, then append all of the command-specific
+     * options. */
+    options = xmemdup(global_long_options, sizeof global_long_options);
+    allocated_options = ARRAY_SIZE(global_long_options);
+    n_options = n_global_long_options;
+    for (p = all_commands; p->name; p++) {
+        if (p->options[0]) {
+            char *save_ptr = NULL;
+            char *name;
+            char *s;
+
+            s = xstrdup(p->options);
+            for (name = strtok_r(s, ",", &save_ptr); name != NULL;
+                 name = strtok_r(NULL, ",", &save_ptr)) {
+                char *equals;
+                int has_arg;
+
+                assert(name[0] == '-' && name[1] == '-' && name[2]);
+                name += 2;
+
+                equals = strchr(name, '=');
+                if (equals) {
+                    has_arg = required_argument;
+                    *equals = '\0';
+                } else {
+                    has_arg = no_argument;
+                }
+
+                o = find_option(name, options, n_options);
+                if (o) {
+                    assert(o - options >= n_global_long_options);
+                    assert(o->has_arg == has_arg);
+                } else {
+                    o = add_option(&options, &n_options, &allocated_options);
+                    o->name = xstrdup(name);
+                    o->has_arg = has_arg;
+                    o->flag = NULL;
+                    o->val = OPT_LOCAL;
+                }
+            }
+
+            free(s);
+        }
+    }
+    o = add_option(&options, &n_options, &allocated_options);
+    memset(o, 0, sizeof *o);
+
     table_style.format = TF_LIST;
 
     for (;;) {
+        int idx;
         int c;
 
-        c = getopt_long(argc, argv, short_options, long_options, NULL);
+        c = getopt_long(argc, argv, short_options, options, &idx);
         if (c == -1) {
             break;
         }
@@ -273,6 +360,16 @@ parse_options(int argc, char *argv[])
             dry_run = true;
             break;
 
+        case OPT_LOCAL:
+            if (shash_find(local_options, options[idx].name)) {
+                vsctl_fatal("'%s' option specified multiple times",
+                            options[idx].name);
+            }
+            shash_add_nocopy(local_options,
+                             xasprintf("--%s", options[idx].name),
+                             optarg ? xstrdup(optarg) : NULL);
+            break;
+
         case 'h':
             usage();
 
@@ -309,10 +406,16 @@ parse_options(int argc, char *argv[])
     if (!db) {
         db = default_db();
     }
+
+    for (i = n_global_long_options; options[i].name; i++) {
+        free(CONST_CAST(char *, options[i].name));
+    }
+    free(options);
 }
 
 static struct vsctl_command *
-parse_commands(int argc, char *argv[], size_t *n_commandsp)
+parse_commands(int argc, char *argv[], struct shash *local_options,
+               size_t *n_commandsp)
 {
     struct vsctl_command *commands;
     size_t n_commands, allocated_commands;
@@ -333,8 +436,10 @@ parse_commands(int argc, char *argv[], size_t *n_commandsp)
                         shash_moved(&c->options);
                     }
                 }
-                parse_command(i - start, &argv[start],
+                parse_command(i - start, &argv[start], local_options,
                               &commands[n_commands++]);
+            } else if (!shash_is_empty(local_options)) {
+                vsctl_fatal("missing command name (use --help for help)");
             }
             start = i + 1;
         }
@@ -347,7 +452,8 @@ parse_commands(int argc, char *argv[], size_t *n_commandsp)
 }
 
 static void
-parse_command(int argc, char *argv[], struct vsctl_command *command)
+parse_command(int argc, char *argv[], struct shash *local_options,
+              struct vsctl_command *command)
 {
     const struct vsctl_command_syntax *p;
     struct shash_node *node;
@@ -355,6 +461,7 @@ parse_command(int argc, char *argv[], struct vsctl_command *command)
     int i;
 
     shash_init(&command->options);
+    shash_swap(local_options, &command->options);
     for (i = 0; i < argc; i++) {
         const char *option = argv[i];
         const char *equals;
@@ -379,7 +486,7 @@ parse_command(int argc, char *argv[], struct vsctl_command *command)
         shash_add_nocopy(&command->options, key, value);
     }
     if (i == argc) {
-        vsctl_fatal("missing command name");
+        vsctl_fatal("missing command name (use --help for help)");
     }
 
     p = find_command(argv[i]);