[ovs-dev] Bug#691508: [PATCH] ovs-vsctl: Allow command-specific options to mingle with global options.

Adam Heath doogie at brainfood.com
Mon Oct 29 20:03:13 UTC 2012


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: 691508 at bugs.debian.org
> Reported-by: Adam Heath <doogie at brainfood.com>
> Signed-off-by: Ben Pfaff <blp at 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           ucko at debian.org
> +Adam Heath              doogie at brainfood.com
>  Ahmed Bilal             numan252 at gmail.com
>  Alan Shieh              ashieh at nicira.com
>  Alban Browaeys          prahal at 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]);



More information about the dev mailing list