[ovs-dev] [PATCH] ovs-vsctl: Add --if-exists option to many database commands.

Gurucharan Shetty shettyg at nicira.com
Wed Jan 16 00:57:27 UTC 2013


On Tue, Jan 15, 2013 at 1:24 PM, Ben Pfaff <blp at nicira.com> wrote:
> A few ovs-vsctl commands have accepted --if-exists options for some time,
> to make it possible to execute them in cases where it doesn't really
> matter if the records they touch exist.  This commit adds this option to
> other commands.
>
> This is intended for initial use with "ovs-vsctl set interface <iface>
> ofport_request=<number>" commands in ovs-ctl for upgrades from OVS 1.9
> to later versions.

There is a whitespace error. Otherwise looks good. Thanks. I will
re-spin my patches.

~Guru


>
> CC: Gurucharan Shetty <gshetty at nicira.com>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  NEWS                     |    2 ++
>  tests/ovs-vsctl.at       |   24 ++++++++++++++
>  utilities/ovs-vsctl.8.in |   48 +++++++++++++++++++++-------
>  utilities/ovs-vsctl.c    |   78 ++++++++++++++++++++++++++++++----------------
>  4 files changed, 114 insertions(+), 38 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 3f06a7a..a7c8a0e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -7,6 +7,8 @@ post-v1.9.0
>        separately on a per-port basis, so it should no longer be
>        possible for a large number of new flows arriving on one port to
>        prevent new flows from being processed on other ports.
> +    - Many "ovs-vsctl" database commands now accept an --if-exists option.
> +      Please refer to the ovs-vsctl manpage for details.
>      - New "vlog/disable-rate-limit" and "vlog/enable-rate-limit" commands
>        available through ovs-appctl allow control over logging rate limits.
>      - The OpenFlow "dp_desc" may now be configured by setting the value of
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index 6a1cc35..326d5a4 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -666,6 +666,18 @@ AT_CHECK([RUN_OVS_VSCTL_TOGETHER([destroy b br0],
>    [0], [stdout], [], [OVS_VSCTL_CLEANUP])
>  AT_CHECK([RUN_OVS_VSCTL([list b])],
>    [0], [], [], [OVS_VSCTL_CLEANUP])
> +AT_CHECK([RUN_OVS_VSCTL([--if-exists get b x datapath_id])],
> +  [0], [], [], [OVS_VSCTL_CLEANUP])
> +AT_CHECK([RUN_OVS_VSCTL([--if-exists list b x])],
> +  [0], [], [], [OVS_VSCTL_CLEANUP])
> +AT_CHECK([RUN_OVS_VSCTL([--if-exists set controller x connection_mode=standalone])],
> +  [0], [], [], [OVS_VSCTL_CLEANUP])
> +AT_CHECK(
> +  [RUN_OVS_VSCTL([--if-exists remove netflow x targets '"1.2.3.4:567"'])],
> +  [0], [], [], [OVS_VSCTL_CLEANUP])
> +AT_CHECK(
> +  [RUN_OVS_VSCTL([--if-exists clear netflow x targets])],
> +  [0], [], [], [OVS_VSCTL_CLEANUP])
>  OVS_VSCTL_CLEANUP
>  AT_CLEANUP
>
> @@ -713,6 +725,9 @@ AT_CHECK([RUN_OVS_VSCTL([list interx x])],
>  AT_CHECK([RUN_OVS_VSCTL([list b x])],
>    [1], [], [ovs-vsctl: no row "x" in table Bridge
>  ], [OVS_VSCTL_CLEANUP])
> +AT_CHECK([RUN_OVS_VSCTL([get b x datapath_id])],
> +  [1], [], [ovs-vsctl: no row "x" in table Bridge
> +], [OVS_VSCTL_CLEANUP])
>  AT_CHECK([RUN_OVS_VSCTL([get b br0 d])],
>    [1], [], [ovs-vsctl: Bridge contains more than one column whose name matches "d"
>  ], [OVS_VSCTL_CLEANUP])
> @@ -728,6 +743,9 @@ AT_CHECK([RUN_OVS_VSCTL([get b br0 datapath_id:y=z])],
>  AT_CHECK([RUN_OVS_VSCTL([set b br0 'datapath_id:y>=z'])],
>    [1], [], [ovs-vsctl: datapath_id:y>=z: argument does not end in "=" followed by a value.
>  ], [OVS_VSCTL_CLEANUP])
> +AT_CHECK([RUN_OVS_VSCTL([set controller x connection_mode=standalone])],
> +  [1], [], [ovs-vsctl: no row "x" in table Controller
> +], [OVS_VSCTL_CLEANUP])
>  AT_CHECK([RUN_OVS_VSCTL([wait-until b br0 datapath_id:y,z])],
>    [1], [], [ovs-vsctl: datapath_id:y,z: argument does not end in "=", "!=", "<", ">", "<=", ">=", "{=}", "{!=}", "{<}", "{>}", "{<=}", or "{>=}" followed by a value.
>  ], [OVS_VSCTL_CLEANUP])
> @@ -758,6 +776,12 @@ AT_CHECK([RUN_OVS_VSCTL([add b br1 datapath_id x y])],
>  AT_CHECK([RUN_OVS_VSCTL([remove netflow `cat netflow-uuid` targets '"1.2.3.4:567"'])],
>    [1], [], [ovs-vsctl: "remove" operation would put 0 values in column targets of table NetFlow but the minimum number is 1
>  ], [OVS_VSCTL_CLEANUP])
> +AT_CHECK([RUN_OVS_VSCTL([remove netflow x targets '"1.2.3.4:567"'])],
> +  [1], [], [ovs-vsctl: no row "x" in table NetFlow
> +], [OVS_VSCTL_CLEANUP])
> +AT_CHECK([RUN_OVS_VSCTL([clear netflow x targets])],
> +  [1], [], [ovs-vsctl: no row "x" in table NetFlow
> +], [OVS_VSCTL_CLEANUP])
>  AT_CHECK([RUN_OVS_VSCTL([clear netflow `cat netflow-uuid` targets])],
>    [1], [], [ovs-vsctl: "clear" operation cannot be applied to column targets of table NetFlow, which is not allowed to be empty
>  ], [OVS_VSCTL_CLEANUP])
> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> index 30baafd..cf6cf88 100644
> --- a/utilities/ovs-vsctl.8.in
> +++ b/utilities/ovs-vsctl.8.in
> @@ -567,13 +567,19 @@ well (but use quotes to prevent the shell from expanding
>  other-config=1=y\fR, which may not have the desired effect).
>  .
>  .ST "Database Command Syntax"
> -.IP "[\fB\-\-columns=\fIcolumn\fR[\fB,\fIcolumn\fR]...] \fBlist \fItable \fR[\fIrecord\fR]..."
> +.
> +.IP "[\fB\-\-if\-exists\fR] [\fB\-\-columns=\fIcolumn\fR[\fB,\fIcolumn\fR]...] \fBlist \fItable \fR[\fIrecord\fR]..."
>  Lists the data in each specified \fIrecord\fR.  If no
>  records are specified, lists all the records in \fItable\fR.
>  .IP
>  If \fB\-\-columns\fR is specified, only the requested columns are
>  listed, in the specified order.  Otherwise, all columns are listed, in
>  alphabetical order by column name.
> +.IP
> +Without \fB\-\-if-exists\fR, it is an error if any specified
> +\fIrecord\fR does not exist.  With \fB\-\-if-exists\fR, the command
> +ignores any \fIrecord\fR that does not exist, without producing any
> +output.
>  .
>  .IP "[\fB\-\-columns=\fIcolumn\fR[\fB,\fIcolumn\fR]...] \fBfind \fItable \fR[\fIcolumn\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR]..."
>  Lists the data in each record in \fItable\fR whose \fIcolumn\fR equals
> @@ -636,16 +642,16 @@ alphabetical order by column name.
>  The UUIDs shown for rows created in the same \fBovs\-vsctl\fR
>  invocation will be wrong.
>  .
> -.IP "[\fB\-\-id=@\fIname\fR] [\fB\-\-if\-exists\fR] \fBget \fItable record \fR[\fIcolumn\fR[\fB:\fIkey\fR]]..."
> +.IP "[\fB\-\-if\-exists\fR] [\fB\-\-id=@\fIname\fR] \fBget \fItable record \fR[\fIcolumn\fR[\fB:\fIkey\fR]]..."
>  Prints the value of each specified \fIcolumn\fR in the given
>  \fIrecord\fR in \fItable\fR.  For map columns, a \fIkey\fR may
>  optionally be specified, in which case the value associated with
>  \fIkey\fR in the column is printed, instead of the entire map.
>  .IP
> -For a map column, without \fB\-\-if\-exists\fR it is an error if
> -\fIkey\fR does not exist; with it, a blank line is printed.  If
> -\fIcolumn\fR is not a map column or if \fIkey\fR is not specified,
> -\fB\-\-if\-exists\fR has no effect.
> +Without \fB\-\-if\-exists\fR, it is an error if \fIrecord\fR does not
> +exist or \fIkey\fR is specified, if \fIkey\fR does not exist in
> +\fIrecord\fR.  With \fB\-\-if\-exists\fR, a missing \fIrecord\fR
> +yields no output and a missing \fIkey\fR prints a blank line.
>  .IP
>  If \fB@\fIname\fR is specified, then the UUID for \fIrecord\fR may be
>  referred to by that name later in the same \fBovs\-vsctl\fR
> @@ -655,24 +661,34 @@ Both \fB\-\-id\fR and the \fIcolumn\fR arguments are optional, but
>  usually at least one or the other should be specified.  If both are
>  omitted, then \fBget\fR has no effect except to verify that
>  \fIrecord\fR exists in \fItable\fR.
> +.IP
> +\fB\-\-id\fR and \fB\-\-if\-exists\fR cannot be used together.
>  .
> -.IP "\fBset \fItable record column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..."
> +.IP "[\fB\-\-if\-exists\fR] \fBset \fItable record column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..."
>  Sets the value of each specified \fIcolumn\fR in the given
>  \fIrecord\fR in \fItable\fR to \fIvalue\fR.  For map columns, a
>  \fIkey\fR may optionally be specified, in which case the value
>  associated with \fIkey\fR in that column is changed (or added, if none
>  exists), instead of the entire map.
> +.IP
> +Without \fB\-\-if-exists\fR, it is an error if \fIrecord\fR does not
> +exist.  With \fB\-\-if-exists\fR, this command does nothing if
> +\fIrecord\fR does not exist.
>  .
> -.IP "\fBadd \fItable record column \fR[\fIkey\fB=\fR]\fIvalue\fR..."
> +.IP "[\fB\-\-if\-exists\fR] \fBadd \fItable record column \fR[\fIkey\fB=\fR]\fIvalue\fR..."
>  Adds the specified value or key-value pair to \fIcolumn\fR in
>  \fIrecord\fR in \fItable\fR.  If \fIcolumn\fR is a map, then \fIkey\fR
>  is required, otherwise it is prohibited.  If \fIkey\fR already exists
>  in a map column, then the current \fIvalue\fR is not replaced (use the
>  \fBset\fR command to replace an existing value).
> +.IP
> +Without \fB\-\-if-exists\fR, it is an error if \fIrecord\fR does not
> +exist.  With \fB\-\-if-exists\fR, this command does nothing if
> +\fIrecord\fR does not exist.
>  .
> -.IP "\fBremove \fItable record column \fR\fIvalue\fR..."
> -.IQ "\fBremove \fItable record column \fR\fIkey\fR..."
> -.IQ "\fBremove \fItable record column \fR\fIkey\fB=\fR\fIvalue\fR..."
> +.IP "[\fB\-\-if\-exists\fR] \fBremove \fItable record column \fR\fIvalue\fR..."
> +.IQ "[\fB\-\-if\-exists\fR] \fBremove \fItable record column \fR\fIkey\fR..."
> +.IQ "[\fB\-\-if\-exists\fR] \fBremove \fItable record column \fR\fIkey\fB=\fR\fIvalue\fR..."
>  Removes the specified values or key-value pairs from \fIcolumn\fR in
>  \fIrecord\fR in \fItable\fR.  The first form applies to columns that
>  are not maps: each specified \fIvalue\fR is removed from the column.
> @@ -683,11 +699,19 @@ pair is removed only if both key and value match.
>  .IP
>  It is not an error if the column does not contain the specified key or
>  value or pair.
> +.IP
> +Without \fB\-\-if-exists\fR, it is an error if \fIrecord\fR does not
> +exist.  With \fB\-\-if-exists\fR, this command does nothing if
> +\fIrecord\fR does not exist.
>  .
> -.IP "\fBclear\fR \fItable record column\fR..."
> +.IP "[\fB\-\-if\-exists\fR] \fBclear\fR \fItable record column\fR..."
>  Sets each \fIcolumn\fR in \fIrecord\fR in \fItable\fR to the empty set
>  or empty map, as appropriate.  This command applies only to columns
>  that are allowed to be empty.
> +.IP
> +Without \fB\-\-if-exists\fR, it is an error if \fIrecord\fR does not
> +exist.  With \fB\-\-if-exists\fR, this command does nothing if
> +\fIrecord\fR does not exist.
>  .
>  .IP "[\fB\-\-id=@\fIname\fR] \fBcreate\fR \fItable column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..."
>  Creates a new record in \fItable\fR and sets the initial values of
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index bccb2c9..bbaaa1b 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -2646,7 +2646,8 @@ get_row_by_id(struct vsctl_context *ctx, const struct vsctl_table_class *table,
>
>  static const struct ovsdb_idl_row *
>  get_row (struct vsctl_context *ctx,
> -         const struct vsctl_table_class *table, const char *record_id)
> +         const struct vsctl_table_class *table, const char *record_id,
> +         bool must_exist)
>  {
>      const struct ovsdb_idl_row *row;
>      struct uuid uuid;
> @@ -2663,15 +2664,7 @@ get_row (struct vsctl_context *ctx,
>              }
>          }
>      }
> -    return row;
> -}
> -
> -static const struct ovsdb_idl_row *
> -must_get_row(struct vsctl_context *ctx,
> -             const struct vsctl_table_class *table, const char *record_id)
> -{
> -    const struct ovsdb_idl_row *row = get_row(ctx, table, record_id);
> -    if (!row) {
> +    if (must_exist && !row) {
>          vsctl_fatal("no row \"%s\" in table %s",
>                      record_id, table->class->name);
>      }
> @@ -2943,7 +2936,7 @@ static void
>  cmd_get(struct vsctl_context *ctx)
>  {
>      const char *id = shash_find_data(&ctx->options, "--id");
> -    bool if_exists = shash_find(&ctx->options, "--if-exists");
> +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
>      const char *table_name = ctx->argv[1];
>      const char *record_id = ctx->argv[2];
>      const struct vsctl_table_class *table;
> @@ -2951,8 +2944,16 @@ cmd_get(struct vsctl_context *ctx)
>      struct ds *out = &ctx->output;
>      int i;
>
> +    if (id && !must_exist) {
> +        vsctl_fatal("--if-exists and --id may not be specified together");
> +    }
> +
>      table = get_table(table_name);
> -    row = must_get_row(ctx, table, record_id);
> +    row = get_row(ctx, table, record_id, must_exist);
> +    if (!row) {
> +        return;
> +    }
> +
>      if (id) {
>          struct ovsdb_symbol *symbol;
>          bool new;
> @@ -3004,7 +3005,7 @@ cmd_get(struct vsctl_context *ctx)
>              idx = ovsdb_datum_find_key(datum, &key,
>                                         column->type.key.type);
>              if (idx == UINT_MAX) {
> -                if (!if_exists) {
> +                if (must_exist) {
>                      vsctl_fatal("no key \"%s\" in %s record \"%s\" column %s",
>                                  key_string, table->class->name, record_id,
>                                  column->name);
> @@ -3130,6 +3131,10 @@ list_record(const struct ovsdb_idl_row *row,
>  {
>      size_t i;
>
> +    if (!row) {
> +        return;
> +    }
> +
>      table_add_row(out);
>      for (i = 0; i < n_columns; i++) {
>          const struct ovsdb_idl_column *column = columns[i];
> @@ -3160,6 +3165,7 @@ static void
>  cmd_list(struct vsctl_context *ctx)
>  {
>      const char *column_names = shash_find_data(&ctx->options, "--columns");
> +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
>      const struct ovsdb_idl_column **columns;
>      const char *table_name = ctx->argv[1];
>      const struct vsctl_table_class *table;
> @@ -3172,7 +3178,7 @@ cmd_list(struct vsctl_context *ctx)
>      out = ctx->table = list_make_table(columns, n_columns);
>      if (ctx->argc > 2) {
>          for (i = 2; i < ctx->argc; i++) {
> -            list_record(must_get_row(ctx, table, ctx->argv[i]),
> +            list_record(get_row(ctx, table, ctx->argv[i], must_exist),
>                          columns, n_columns, out);
>          }
>      } else {
> @@ -3302,6 +3308,7 @@ set_column(const struct vsctl_table_class *table,
>  static void
>  cmd_set(struct vsctl_context *ctx)
>  {
> +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
>      const char *table_name = ctx->argv[1];
>      const char *record_id = ctx->argv[2];
>      const struct vsctl_table_class *table;
> @@ -3309,7 +3316,11 @@ cmd_set(struct vsctl_context *ctx)
>      int i;
>
>      table = get_table(table_name);
> -    row = must_get_row(ctx, table, record_id);
> +    row = get_row(ctx, table, record_id, must_exist);
> +    if (!row) {
> +        return;
> +    }
> +
>      for (i = 3; i < ctx->argc; i++) {
>          set_column(table, row, ctx->argv[i], ctx->symtab);
>      }
> @@ -3333,6 +3344,7 @@ pre_cmd_add(struct vsctl_context *ctx)
>  static void
>  cmd_add(struct vsctl_context *ctx)
>  {
> +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
>      const char *table_name = ctx->argv[1];
>      const char *record_id = ctx->argv[2];
>      const char *column_name = ctx->argv[3];
> @@ -3344,8 +3356,11 @@ cmd_add(struct vsctl_context *ctx)
>      int i;
>
>      table = get_table(table_name);
> -    row = must_get_row(ctx, table, record_id);
>      die_if_error(get_column(table, column_name, &column));
> +    row = get_row(ctx, table, record_id, must_exist);
> +    if (!row) {
> +        return;
> +    }
>
>      type = &column->type;
>      ovsdb_datum_clone(&old, ovsdb_idl_read(row, column), &column->type);
> @@ -3390,6 +3405,7 @@ pre_cmd_remove(struct vsctl_context *ctx)
>  static void
>  cmd_remove(struct vsctl_context *ctx)
>  {
> +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
>      const char *table_name = ctx->argv[1];
>      const char *record_id = ctx->argv[2];
>      const char *column_name = ctx->argv[3];
> @@ -3401,8 +3417,11 @@ cmd_remove(struct vsctl_context *ctx)
>      int i;
>
>      table = get_table(table_name);
> -    row = must_get_row(ctx, table, record_id);
>      die_if_error(get_column(table, column_name, &column));
> +    row = get_row(ctx, table, record_id, must_exist);
> +    if (!row) {
> +        return;
> +    }
>
>      type = &column->type;
>      ovsdb_datum_clone(&old, ovsdb_idl_read(row, column), &column->type);
> @@ -3457,6 +3476,7 @@ pre_cmd_clear(struct vsctl_context *ctx)
>  static void
>  cmd_clear(struct vsctl_context *ctx)
>  {
> +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
>      const char *table_name = ctx->argv[1];
>      const char *record_id = ctx->argv[2];
>      const struct vsctl_table_class *table;
> @@ -3464,7 +3484,11 @@ cmd_clear(struct vsctl_context *ctx)
>      int i;
>
>      table = get_table(table_name);
> -    row = must_get_row(ctx, table, record_id);
> +    row = get_row(ctx, table, record_id, must_exist);
> +    if (!row) {
> +        return;
> +    }
> +
>      for (i = 3; i < ctx->argc; i++) {
>          const struct ovsdb_idl_column *column;
>          const struct ovsdb_type *type;
> @@ -3597,7 +3621,7 @@ cmd_destroy(struct vsctl_context *ctx)
>          for (i = 2; i < ctx->argc; i++) {
>              const struct ovsdb_idl_row *row;
>
> -            row = (must_exist ? must_get_row : get_row)(ctx, table, ctx->argv[i]);
> +            row = get_row(ctx, table, ctx->argv[i], must_exist);
>              if (row) {
>                  ovsdb_idl_txn_delete(row);
>              }
> @@ -3778,7 +3802,7 @@ cmd_wait_until(struct vsctl_context *ctx)
>
>      table = get_table(table_name);
>
> -    row = get_row(ctx, table, record_id);
> +    row = get_row(ctx, table, record_id, false);
>      if (!row) {
>          ctx->try_again = true;
>          return;
> @@ -4126,12 +4150,14 @@ static const struct vsctl_command_syntax all_commands[] = {
>      /* Database commands. */
>      {"comment", 0, INT_MAX, NULL, NULL, NULL, "", RO},
>      {"get", 2, INT_MAX, pre_cmd_get, cmd_get, NULL, "--if-exists,--id=", RO},
> -    {"list", 1, INT_MAX, pre_cmd_list, cmd_list, NULL, "--columns=", RO},
> +    {"list", 1, INT_MAX, pre_cmd_list, cmd_list, NULL,
> +     "--if-exists,--columns=", RO},
>      {"find", 1, INT_MAX, pre_cmd_find, cmd_find, NULL, "--columns=", RO},
> -    {"set", 3, INT_MAX, pre_cmd_set, cmd_set, NULL, "", RW},
> -    {"add", 4, INT_MAX, pre_cmd_add, cmd_add, NULL, "", RW},
> -    {"remove", 4, INT_MAX, pre_cmd_remove, cmd_remove, NULL, "", RW},
> -    {"clear", 3, INT_MAX, pre_cmd_clear, cmd_clear, NULL, "", RW},
> +    {"set", 3, INT_MAX, pre_cmd_set, cmd_set, NULL, "--if-exists", RW},
> +    {"add", 4, INT_MAX, pre_cmd_add, cmd_add, NULL, "--if-exists", RW},
> +    {"remove", 4, INT_MAX, pre_cmd_remove, cmd_remove, NULL, "--if-exists",
> +     RW},
> +    {"clear", 3, INT_MAX, pre_cmd_clear, cmd_clear, NULL, "--if-exists", RW},
>      {"create", 2, INT_MAX, pre_create, cmd_create, post_create, "--id=", RW},
>      {"destroy", 1, INT_MAX, pre_cmd_destroy, cmd_destroy, NULL,
>       "--if-exists,--all", RW},
> --
> 1.7.10.4
>



More information about the dev mailing list