On 10/29/2012 11:34 AM, Ben Pfaff wrote: > 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.
Tbh, I would actually prefer to have command-specific options that appear in the global area issue an error. == one-vsctl: Found command-specific --may-exist in global area, please use -- instead. == See (1) for a reason why I'd prefer it the way I suggested above. 1: http://lwn.net/Articles/520198/ you might need to wait a few days for it to become publically available. > > CC: 691...@bugs.debian.org > Reported-by: Adam Heath <doo...@brainfood.com> > Signed-off-by: Ben Pfaff <b...@nicira.com> > --- > AUTHORS | 1 + > tests/ovs-vsctl.at | 13 ++++- > utilities/ovs-vsctl.8.in | 11 ++-- > utilities/ovs-vsctl.c | 131 +++++++++++++++++++++++++++++++++++++++++---- > 4 files changed, 137 insertions(+), 19 deletions(-) > > diff --git a/AUTHORS b/AUTHORS > index 4687865..83e6bb5 100644 > --- a/AUTHORS > +++ b/AUTHORS > @@ -81,6 +81,7 @@ The following additional people are mentioned in commit > logs as having > provided helpful bug reports or suggestions. > > Aaron M. Ucko u...@debian.org > +Adam Heath doo...@brainfood.com > Ahmed Bilal numan...@gmail.com > Alan Shieh ash...@nicira.com > Alban Browaeys pra...@yahoo.com > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at > index e903619..cb12f31 100644 > --- a/tests/ovs-vsctl.at > +++ b/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 > @@ -655,6 +655,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])], > diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in > index 1b80d05..8f70d6b 100644 > --- a/utilities/ovs-vsctl.8.in > +++ b/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 > -options and following commands. (If the first command does not have > -any options, then the first \fB\-\-\fR may be omitted.) The command > +itself as a command-line argument, to separate it from the following > +commands. (The \fB\-\-\fR before the first command is optional.) The > +command > itself starts with command-specific options, if any, followed by the > command name and any arguments. See \fBEXAMPLES\fR below for syntax > examples. > @@ -769,10 +769,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 > -separate \fBdel\-br\fR's options from the global options): > +Delete bridge \fBbr0\fR if it exists: > .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 > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c > index fda3a89..1745418 100644 > --- a/utilities/ovs-vsctl.c > +++ b/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); > - commands = parse_commands(argc - optind, argv + optind, &n_commands); > + shash_init(&local_options); > + 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]); _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
