From 401d5a6d16438ceb2e09c19677b97504593b4b51 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 10 Dec 2012 14:24:36 -0800 Subject: [PATCH] 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 Signed-off-by: Ben Pfaff Acked-by: Kyle Mestery --- 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 7f3f32644..ca02ca0ce 100644 --- a/AUTHORS +++ b/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 diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at index 9552469db..6a1cc3567 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 @@ -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])], diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in index 79269db76..e3ca78b35 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. @@ -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 -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 69e37e002..bccb2c921 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]); -- 2.43.0