[ovs-dev,4/4] ovn-nbctl: Enable database commands using db-ctl-base infrastructure.

This makes ovn-nbctl into a pretty slavish imitation of ovn-nbctl, using
almost the same code.  It has two immediate benefits.  First, multiple
commands can now be chained together into a single ovn-nbctl invocation.
Second, the database commands such as "create", "set", and so on allow
queries and modifications that don't have specific commands already.
In the following commit, this allows testing the OVN ACL feature.

Signed-off-by: Ben Pfaff <blp@nicira.com>
---
 ovn/utilities/ovn-nbctl.8.xml |    4 +-
 ovn/utilities/ovn-nbctl.c     | 1043 ++++++++++++++++++++++++-----------------
 tests/ovn-nbctl.at            |    4 +-
 3 files changed, 608 insertions(+), 443 deletions(-)

On 11 September 2015 at 20:29, Ben Pfaff <blp@nicira.com> wrote:

> This makes ovn-nbctl into a pretty slavish imitation of ovn-nbctl, using
> almost the same code.  It has two immediate benefits.  First, multiple
> commands can now be chained together into a single ovn-nbctl invocation.
> Second, the database commands such as "create", "set", and so on allow
> queries and modifications that don't have specific commands already.
> In the following commit, this allows testing the OVN ACL feature.
>
> Signed-off-by: Ben Pfaff <blp@nicira.com>
>




imitation of ovn-sbctl?

Also,

one thing I noticed is that there is no implementation
for 'struct nbctl_context' and 'cache'.  so, there is no
guarantee that in a long cmd chain, the later cmd
will never see the invalid db states.

Thanks,
Alex Wang,



> ---
>  ovn/utilities/ovn-nbctl.8.xml |    4 +-
>  ovn/utilities/ovn-nbctl.c     | 1043
> ++++++++++++++++++++++++-----------------
>  tests/ovn-nbctl.at            |    4 +-
>  3 files changed, 608 insertions(+), 443 deletions(-)
>
> diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
> index f9f58e5..ad340ec 100644
> --- a/ovn/utilities/ovn-nbctl.8.xml
> +++ b/ovn/utilities/ovn-nbctl.8.xml
> @@ -68,13 +68,13 @@
>
>      <h1>ACL Commands</h1>
>      <dl>
> -      <dt><code>acl-add</code> <var>lswitch</var> <var>direction</var>
> <var>priority</var> <var>match</var> <var>action</var>
> [<code>log</code>]</dt>
> +      <dt>[<code>--log</code>] <code>acl-add</code> <var>lswitch</var>
> <var>direction</var> <var>priority</var> <var>match</var>
> <var>action</var></dt>
>        <dd>
>          Adds the specified ACL to <var>lswitch</var>.
>          <var>direction</var> must be either <code>from-lport</code> or
>          <code>to-lport</code>.  <var>priority</var> must be between
>          <code>1</code> and <code>65534</code>, inclusive.  If
> -        <code>log</code> is supplied, packet logging is enabled for the
> +        <code>--log</code> is specified, packet logging is enabled for the
>          ACL.  A full description of the fields are in
> <code>ovn-nb</code>(5).
>        </dd>
>
> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> index 6eae0e1..c308ddc 100644
> --- a/ovn/utilities/ovn-nbctl.c
> +++ b/ovn/utilities/ovn-nbctl.c
> @@ -20,8 +20,10 @@
>  #include <stdio.h>
>
>  #include "command-line.h"
> +#include "db-ctl-base.h"
>  #include "dirs.h"
>  #include "fatal-signal.h"
> +#include "json.h"
>  #include "ovn/lib/ovn-nb-idl.h"
>  #include "poll-loop.h"
>  #include "process.h"
> @@ -29,19 +31,253 @@
>  #include "stream.h"
>  #include "stream-ssl.h"
>  #include "svec.h"
> +#include "table.h"
> +#include "timeval.h"
>  #include "util.h"
>  #include "openvswitch/vlog.h"
>
> -VLOG_DEFINE_THIS_MODULE(ovn_nbctl);
> +VLOG_DEFINE_THIS_MODULE(nbctl);
>
> -struct nbctl_context {
> +/* --db: The database server to contact. */
> +static const char *db;
> +
> +/* --oneline: Write each command's output as a single line? */
> +static bool oneline;
> +
> +/* --dry-run: Do not commit any changes. */
> +static bool dry_run;
> +
> +/* --timeout: Time to wait for a connection to 'db'. */
> +static int timeout;
> +
> +/* Format for table output. */
> +static struct table_style table_style = TABLE_STYLE_DEFAULT;
> +
> +/* The IDL we're using and the current transaction, if any.
> + * This is for use by nbctl_exit() only, to allow it to clean up.
> + * Other code should use its context arguments. */
> +static struct ovsdb_idl *the_idl;
> +static struct ovsdb_idl_txn *the_idl_txn;
> +OVS_NO_RETURN static void nbctl_exit(int status);
> +
> +static void nbctl_cmd_init(void);
> +OVS_NO_RETURN static void usage(void);
> +static void parse_options(int argc, char *argv[], struct shash
> *local_options);
> +static const char *nbctl_default_db(void);
> +static void run_prerequisites(struct ctl_command[], size_t n_commands,
> +                              struct ovsdb_idl *);
> +static void do_nbctl(const char *args, struct ctl_command *, size_t n,
> +                     struct ovsdb_idl *);
> +
> +int
> +main(int argc, char *argv[])
> +{
> +    extern struct vlog_module VLM_reconnect;
>      struct ovsdb_idl *idl;
> -    struct ovsdb_idl_txn *txn;
> -};
> +    struct ctl_command *commands;
> +    struct shash local_options;
> +    unsigned int seqno;
> +    size_t n_commands;
> +    char *args;
>
> -static const char *db;
> +    set_program_name(argv[0]);
> +    fatal_ignore_sigpipe();
> +    vlog_set_levels(NULL, VLF_CONSOLE, VLL_WARN);
> +    vlog_set_levels(&VLM_reconnect, VLF_ANY_DESTINATION, VLL_WARN);
> +    nbrec_init();
> +
> +    nbctl_cmd_init();
> +
> +    /* Log our arguments.  This is often valuable for debugging systems.
> */
> +    args = process_escape_args(argv);
> +    VLOG(ctl_might_write_to_db(argv) ? VLL_INFO : VLL_DBG,
> +         "Called as %s", args);
> +
> +    /* Parse command line. */
> +    shash_init(&local_options);
> +    parse_options(argc, argv, &local_options);
> +    commands = ctl_parse_commands(argc - optind, argv + optind,
> &local_options,
> +                                  &n_commands);
> +
> +    if (timeout) {
> +        time_alarm(timeout);
> +    }
> +
> +    /* Initialize IDL. */
> +    idl = the_idl = ovsdb_idl_create(db, &nbrec_idl_class, true, false);
> +    run_prerequisites(commands, n_commands, idl);
> +
> +    /* Execute the commands.
> +     *
> +     * 'seqno' is the database sequence number for which we last tried to
> +     * execute our transaction.  There's no point in trying to commit
> more than
> +     * once for any given sequence number, because if the transaction
> fails
> +     * it's because the database changed and we need to obtain an
> up-to-date
> +     * view of the database before we try the transaction again. */
> +    seqno = ovsdb_idl_get_seqno(idl);
> +    for (;;) {
> +        ovsdb_idl_run(idl);
> +        if (!ovsdb_idl_is_alive(idl)) {
> +            int retval = ovsdb_idl_get_last_error(idl);
> +            ctl_fatal("%s: database connection failed (%s)",
> +                        db, ovs_retval_to_string(retval));
> +        }
> +
> +        if (seqno != ovsdb_idl_get_seqno(idl)) {
> +            seqno = ovsdb_idl_get_seqno(idl);
> +            do_nbctl(args, commands, n_commands, idl);
> +        }
>
> -static const char *default_db(void);
> +        if (seqno == ovsdb_idl_get_seqno(idl)) {
> +            ovsdb_idl_wait(idl);
> +            poll_block();
> +        }
> +    }
> +}
> +
> +static const char *
> +nbctl_default_db(void)
> +{
> +    static char *def;
> +    if (!def) {
> +        def = getenv("OVN_NB_DB");
> +        if (!def) {
> +            def = xasprintf("unix:%s/db.sock", ovs_rundir());
> +        }
> +    }
> +    return def;
> +}
> +
> +static void
> +parse_options(int argc, char *argv[], struct shash *local_options)
> +{
> +    enum {
> +        OPT_DB = UCHAR_MAX + 1,
> +        OPT_NO_SYSLOG,
> +        OPT_DRY_RUN,
> +        OPT_ONELINE,
> +        OPT_LOCAL,
> +        OPT_COMMANDS,
> +        OPT_OPTIONS,
> +        VLOG_OPTION_ENUMS,
> +        TABLE_OPTION_ENUMS
> +    };
> +    static const struct option global_long_options[] = {
> +        {"db", required_argument, NULL, OPT_DB},
> +        {"no-syslog", no_argument, NULL, OPT_NO_SYSLOG},
> +        {"dry-run", no_argument, NULL, OPT_DRY_RUN},
> +        {"oneline", no_argument, NULL, OPT_ONELINE},
> +        {"timeout", required_argument, NULL, 't'},
> +        {"help", no_argument, NULL, 'h'},
> +        {"commands", no_argument, NULL, OPT_COMMANDS},
> +        {"options", no_argument, NULL, OPT_OPTIONS},
> +        {"version", no_argument, NULL, 'V'},
> +        VLOG_LONG_OPTIONS,
> +        STREAM_SSL_LONG_OPTIONS,
> +        TABLE_LONG_OPTIONS,
> +        {NULL, 0, NULL, 0},
> +    };
> +    const int n_global_long_options = ARRAY_SIZE(global_long_options) - 1;
> +    char *tmp, *short_options;
> +
> +    struct option *options;
> +    size_t allocated_options;
> +    size_t n_options;
> +    size_t i;
> +
> +    tmp = ovs_cmdl_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;
> +    ctl_add_cmd_options(&options, &n_options, &allocated_options,
> OPT_LOCAL);
> +    table_style.format = TF_LIST;
> +
> +    for (;;) {
> +        int idx;
> +        int c;
> +
> +        c = getopt_long(argc, argv, short_options, options, &idx);
> +        if (c == -1) {
> +            break;
> +        }
> +
> +        switch (c) {
> +        case OPT_DB:
> +            db = optarg;
> +            break;
> +
> +        case OPT_ONELINE:
> +            oneline = true;
> +            break;
> +
> +        case OPT_NO_SYSLOG:
> +            vlog_set_levels(&VLM_nbctl, VLF_SYSLOG, VLL_WARN);
> +            break;
> +
> +        case OPT_DRY_RUN:
> +            dry_run = true;
> +            break;
> +
> +        case OPT_LOCAL:
> +            if (shash_find(local_options, options[idx].name)) {
> +                ctl_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();
> +            exit(EXIT_SUCCESS);
> +
> +        case OPT_COMMANDS:
> +            ctl_print_commands();
> +
> +        case OPT_OPTIONS:
> +            ctl_print_options(global_long_options);
> +
> +        case 'V':
> +            ovs_print_version(0, 0);
> +            printf("DB Schema %s\n", nbrec_get_db_version());
> +            exit(EXIT_SUCCESS);
> +
> +        case 't':
> +            timeout = strtoul(optarg, NULL, 10);
> +            if (timeout < 0) {
> +                ctl_fatal("value %s on -t or --timeout is invalid",
> optarg);
> +            }
> +            break;
> +
> +        VLOG_OPTION_HANDLERS
> +        TABLE_OPTION_HANDLERS(&table_style)
> +        STREAM_SSL_OPTION_HANDLERS
> +
> +        case '?':
> +            exit(EXIT_FAILURE);
> +
> +        default:
> +            abort();
> +        }
> +    }
> +
> +    if (!db) {
> +        db = nbctl_default_db();
> +    }
> +
> +    for (i = n_global_long_options; options[i].name; i++) {
> +        free(CONST_CAST(char *, options[i].name));
> +    }
> +    free(options);
> +}
>
>  static void
>  usage(void)
> @@ -102,18 +338,24 @@ Logical port commands:\n\
>    lport-get-options LPORT   Get the type specific options for LPORT\n\
>  \n\
>  Options:\n\
> -  --db=DATABASE             connect to DATABASE\n\
> -                            (default: %s)\n\
> -  -h, --help                display this help message\n\
> -  -o, --options             list available options\n\
> -  -V, --version             display version information\n\
> -", program_name, program_name, default_db());
> +  --db=DATABASE               connect to DATABASE\n\
> +                              (default: %s)\n\
> +  -t, --timeout=SECS          wait at most SECS seconds\n\
> +  --dry-run                   do not commit changes to database\n\
> +  --oneline                   print exactly one line of output per
> command\n",
> +           program_name, program_name, nbctl_default_db());
>      vlog_usage();
> -    stream_usage("database", true, true, false);
> +    printf("\
> +  --no-syslog             equivalent to --verbose=nbctl:syslog:warn\n");
> +    printf("\n\
> +Other options:\n\
> +  -h, --help                  display this help message\n\
> +  -V, --version               display version information\n");
> +    exit(EXIT_SUCCESS);
>  }
>
>  static const struct nbrec_logical_switch *
> -lswitch_by_name_or_uuid(struct nbctl_context *nb_ctx, const char *id)
> +lswitch_by_name_or_uuid(struct ctl_context *ctx, const char *id)
>  {
>      const struct nbrec_logical_switch *lswitch = NULL;
>      bool is_uuid = false;
> @@ -122,14 +364,14 @@ lswitch_by_name_or_uuid(struct nbctl_context
> *nb_ctx, const char *id)
>
>      if (uuid_from_string(&lswitch_uuid, id)) {
>          is_uuid = true;
> -        lswitch = nbrec_logical_switch_get_for_uuid(nb_ctx->idl,
> +        lswitch = nbrec_logical_switch_get_for_uuid(ctx->idl,
>                                                      &lswitch_uuid);
>      }
>
>      if (!lswitch) {
>          const struct nbrec_logical_switch *iter;
>
> -        NBREC_LOGICAL_SWITCH_FOR_EACH(iter, nb_ctx->idl) {
> +        NBREC_LOGICAL_SWITCH_FOR_EACH(iter, ctx->idl) {
>              if (strcmp(iter->name, id)) {
>                  continue;
>              }
> @@ -153,67 +395,64 @@ lswitch_by_name_or_uuid(struct nbctl_context
> *nb_ctx, const char *id)
>  }
>
>  static void
> -print_lswitch(const struct nbrec_logical_switch *lswitch)
> +print_lswitch(const struct nbrec_logical_switch *lswitch, struct ds *s)
>  {
> -    printf("    lswitch "UUID_FMT" (%s)\n",
> -           UUID_ARGS(&lswitch->header_.uuid), lswitch->name);
> +    ds_put_format(s, "    lswitch "UUID_FMT" (%s)\n",
> +                  UUID_ARGS(&lswitch->header_.uuid), lswitch->name);
>
>      for (size_t i = 0; i < lswitch->n_ports; i++) {
>          const struct nbrec_logical_port *lport = lswitch->ports[i];
>
> -        printf("        lport %s\n", lport->name);
> +        ds_put_format(s, "        lport %s\n", lport->name);
>          if (lport->parent_name && lport->n_tag) {
> -            printf("            parent: %s, tag:%"PRIu64"\n",
> -                   lport->parent_name, lport->tag[0]);
> +            ds_put_format(s, "            parent: %s, tag:%"PRIu64"\n",
> +                          lport->parent_name, lport->tag[0]);
>          }
>          if (lport->n_macs) {
> -            printf("            macs:");
> +            ds_put_cstr(s, "            macs:");
>              for (size_t j = 0; j < lport->n_macs; j++) {
> -                printf(" %s", lport->macs[j]);
> +                ds_put_format(s, " %s", lport->macs[j]);
>              }
> -            printf("\n");
> +            ds_put_char(s, '\n');
>          }
>      }
>  }
>
>  static void
> -nbctl_show(struct ovs_cmdl_context *ctx)
> +nbctl_show(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const struct nbrec_logical_switch *lswitch;
>
>      if (ctx->argc == 2) {
> -        lswitch = lswitch_by_name_or_uuid(nb_ctx, ctx->argv[1]);
> +        lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1]);
>          if (lswitch) {
> -            print_lswitch(lswitch);
> +            print_lswitch(lswitch, &ctx->output);
>          }
>      } else {
> -        NBREC_LOGICAL_SWITCH_FOR_EACH(lswitch, nb_ctx->idl) {
> -            print_lswitch(lswitch);
> +        NBREC_LOGICAL_SWITCH_FOR_EACH(lswitch, ctx->idl) {
> +            print_lswitch(lswitch, &ctx->output);
>          }
>      }
>  }
>
>  static void
> -nbctl_lswitch_add(struct ovs_cmdl_context *ctx)
> +nbctl_lswitch_add(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      struct nbrec_logical_switch *lswitch;
>
> -    lswitch = nbrec_logical_switch_insert(nb_ctx->txn);
> +    lswitch = nbrec_logical_switch_insert(ctx->txn);
>      if (ctx->argc == 2) {
>          nbrec_logical_switch_set_name(lswitch, ctx->argv[1]);
>      }
>  }
>
>  static void
> -nbctl_lswitch_del(struct ovs_cmdl_context *ctx)
> +nbctl_lswitch_del(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_switch *lswitch;
>
> -    lswitch = lswitch_by_name_or_uuid(nb_ctx, id);
> +    lswitch = lswitch_by_name_or_uuid(ctx, id);
>      if (!lswitch) {
>          return;
>      }
> @@ -222,35 +461,33 @@ nbctl_lswitch_del(struct ovs_cmdl_context *ctx)
>  }
>
>  static void
> -nbctl_lswitch_list(struct ovs_cmdl_context *ctx)
> +nbctl_lswitch_list(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const struct nbrec_logical_switch *lswitch;
>      struct smap lswitches;
>
>      smap_init(&lswitches);
> -    NBREC_LOGICAL_SWITCH_FOR_EACH(lswitch, nb_ctx->idl) {
> +    NBREC_LOGICAL_SWITCH_FOR_EACH(lswitch, ctx->idl) {
>          smap_add_format(&lswitches, lswitch->name, UUID_FMT " (%s)",
>                          UUID_ARGS(&lswitch->header_.uuid), lswitch->name);
>      }
>      const struct smap_node **nodes = smap_sort(&lswitches);
>      for (size_t i = 0; i < smap_count(&lswitches); i++) {
>          const struct smap_node *node = nodes[i];
> -        printf("%s\n", node->value);
> +        ds_put_format(&ctx->output, "%s\n", node->value);
>      }
>      smap_destroy(&lswitches);
>      free(nodes);
>  }
>
>  static void
> -nbctl_lswitch_set_external_id(struct ovs_cmdl_context *ctx)
> +nbctl_lswitch_set_external_id(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_switch *lswitch;
>      struct smap new_external_ids;
>
> -    lswitch = lswitch_by_name_or_uuid(nb_ctx, id);
> +    lswitch = lswitch_by_name_or_uuid(ctx, id);
>      if (!lswitch) {
>          return;
>      }
> @@ -267,13 +504,12 @@ nbctl_lswitch_set_external_id(struct
> ovs_cmdl_context *ctx)
>  }
>
>  static void
> -nbctl_lswitch_get_external_id(struct ovs_cmdl_context *ctx)
> +nbctl_lswitch_get_external_id(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_switch *lswitch;
>
> -    lswitch = lswitch_by_name_or_uuid(nb_ctx, id);
> +    lswitch = lswitch_by_name_or_uuid(ctx, id);
>      if (!lswitch) {
>          return;
>      }
> @@ -286,7 +522,7 @@ nbctl_lswitch_get_external_id(struct ovs_cmdl_context
> *ctx)
>
>          value = smap_get(&lswitch->external_ids, key);
>          if (value) {
> -            printf("%s\n", value);
> +            ds_put_format(&ctx->output, "%s\n", value);
>          }
>      } else {
>          struct smap_node *node;
> @@ -294,13 +530,13 @@ nbctl_lswitch_get_external_id(struct
> ovs_cmdl_context *ctx)
>          /* List all external IDs */
>
>          SMAP_FOR_EACH(node, &lswitch->external_ids) {
> -            printf("%s=%s\n", node->key, node->value);
> +            ds_put_format(&ctx->output, "%s=%s\n", node->key,
> node->value);
>          }
>      }
>  }
>
>  static const struct nbrec_logical_port *
> -lport_by_name_or_uuid(struct nbctl_context *nb_ctx, const char *id)
> +lport_by_name_or_uuid(struct ctl_context *ctx, const char *id)
>  {
>      const struct nbrec_logical_port *lport = NULL;
>      bool is_uuid = false;
> @@ -308,11 +544,11 @@ lport_by_name_or_uuid(struct nbctl_context *nb_ctx,
> const char *id)
>
>      if (uuid_from_string(&lport_uuid, id)) {
>          is_uuid = true;
> -        lport = nbrec_logical_port_get_for_uuid(nb_ctx->idl, &lport_uuid);
> +        lport = nbrec_logical_port_get_for_uuid(ctx->idl, &lport_uuid);
>      }
>
>      if (!lport) {
> -        NBREC_LOGICAL_PORT_FOR_EACH(lport, nb_ctx->idl) {
> +        NBREC_LOGICAL_PORT_FOR_EACH(lport, ctx->idl) {
>              if (!strcmp(lport->name, id)) {
>                  break;
>              }
> @@ -328,14 +564,13 @@ lport_by_name_or_uuid(struct nbctl_context *nb_ctx,
> const char *id)
>  }
>
>  static void
> -nbctl_lport_add(struct ovs_cmdl_context *ctx)
> +nbctl_lport_add(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      struct nbrec_logical_port *lport;
>      const struct nbrec_logical_switch *lswitch;
>      int64_t tag;
>
> -    lswitch = lswitch_by_name_or_uuid(nb_ctx, ctx->argv[1]);
> +    lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1]);
>      if (!lswitch) {
>          return;
>      }
> @@ -355,7 +590,7 @@ nbctl_lport_add(struct ovs_cmdl_context *ctx)
>      }
>
>      /* Create the logical port. */
> -    lport = nbrec_logical_port_insert(nb_ctx->txn);
> +    lport = nbrec_logical_port_insert(ctx->txn);
>      nbrec_logical_port_set_name(lport, ctx->argv[2]);
>      if (ctx->argc == 5) {
>          nbrec_logical_port_set_parent_name(lport, ctx->argv[3]);
> @@ -395,19 +630,18 @@ remove_lport(const struct nbrec_logical_switch
> *lswitch, size_t idx)
>  }
>
>  static void
> -nbctl_lport_del(struct ovs_cmdl_context *ctx)
> +nbctl_lport_del(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const struct nbrec_logical_port *lport;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, ctx->argv[1]);
> +    lport = lport_by_name_or_uuid(ctx, ctx->argv[1]);
>      if (!lport) {
>          return;
>      }
>
>      /* Find the switch that contains 'lport', then delete it. */
>      const struct nbrec_logical_switch *lswitch;
> -    NBREC_LOGICAL_SWITCH_FOR_EACH (lswitch, nb_ctx->idl) {
> +    NBREC_LOGICAL_SWITCH_FOR_EACH (lswitch, ctx->idl) {
>          for (size_t i = 0; i < lswitch->n_ports; i++) {
>              if (lswitch->ports[i] == lport) {
>                  remove_lport(lswitch, i);
> @@ -421,15 +655,14 @@ nbctl_lport_del(struct ovs_cmdl_context *ctx)
>  }
>
>  static void
> -nbctl_lport_list(struct ovs_cmdl_context *ctx)
> +nbctl_lport_list(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_switch *lswitch;
>      struct smap lports;
>      size_t i;
>
> -    lswitch = lswitch_by_name_or_uuid(nb_ctx, id);
> +    lswitch = lswitch_by_name_or_uuid(ctx, id);
>      if (!lswitch) {
>          return;
>      }
> @@ -443,53 +676,50 @@ nbctl_lport_list(struct ovs_cmdl_context *ctx)
>      const struct smap_node **nodes = smap_sort(&lports);
>      for (i = 0; i < smap_count(&lports); i++) {
>          const struct smap_node *node = nodes[i];
> -        printf("%s\n", node->value);
> +        ds_put_format(&ctx->output, "%s\n", node->value);
>      }
>      smap_destroy(&lports);
>      free(nodes);
>  }
>
>  static void
> -nbctl_lport_get_parent(struct ovs_cmdl_context *ctx)
> +nbctl_lport_get_parent(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const struct nbrec_logical_port *lport;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, ctx->argv[1]);
> +    lport = lport_by_name_or_uuid(ctx, ctx->argv[1]);
>      if (!lport) {
>          return;
>      }
>
>      if (lport->parent_name) {
> -        printf("%s\n", lport->parent_name);
> +        ds_put_format(&ctx->output, "%s\n", lport->parent_name);
>      }
>  }
>
>  static void
> -nbctl_lport_get_tag(struct ovs_cmdl_context *ctx)
> +nbctl_lport_get_tag(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const struct nbrec_logical_port *lport;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, ctx->argv[1]);
> +    lport = lport_by_name_or_uuid(ctx, ctx->argv[1]);
>      if (!lport) {
>          return;
>      }
>
>      if (lport->n_tag > 0) {
> -        printf("%"PRId64"\n", lport->tag[0]);
> +        ds_put_format(&ctx->output, "%"PRId64"\n", lport->tag[0]);
>      }
>  }
>
>  static void
> -nbctl_lport_set_external_id(struct ovs_cmdl_context *ctx)
> +nbctl_lport_set_external_id(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_port *lport;
>      struct smap new_external_ids;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, id);
> +    lport = lport_by_name_or_uuid(ctx, id);
>      if (!lport) {
>          return;
>      }
> @@ -506,13 +736,12 @@ nbctl_lport_set_external_id(struct ovs_cmdl_context
> *ctx)
>  }
>
>  static void
> -nbctl_lport_get_external_id(struct ovs_cmdl_context *ctx)
> +nbctl_lport_get_external_id(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_port *lport;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, id);
> +    lport = lport_by_name_or_uuid(ctx, id);
>      if (!lport) {
>          return;
>      }
> @@ -525,7 +754,7 @@ nbctl_lport_get_external_id(struct ovs_cmdl_context
> *ctx)
>
>          value = smap_get(&lport->external_ids, key);
>          if (value) {
> -            printf("%s\n", value);
> +            ds_put_format(&ctx->output, "%s\n", value);
>          }
>      } else {
>          struct smap_node *node;
> @@ -533,19 +762,18 @@ nbctl_lport_get_external_id(struct ovs_cmdl_context
> *ctx)
>          /* List all external IDs */
>
>          SMAP_FOR_EACH(node, &lport->external_ids) {
> -            printf("%s=%s\n", node->key, node->value);
> +            ds_put_format(&ctx->output, "%s=%s\n", node->key,
> node->value);
>          }
>      }
>  }
>
>  static void
> -nbctl_lport_set_macs(struct ovs_cmdl_context *ctx)
> +nbctl_lport_set_macs(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_port *lport;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, id);
> +    lport = lport_by_name_or_uuid(ctx, id);
>      if (!lport) {
>          return;
>      }
> @@ -555,16 +783,15 @@ nbctl_lport_set_macs(struct ovs_cmdl_context *ctx)
>  }
>
>  static void
> -nbctl_lport_get_macs(struct ovs_cmdl_context *ctx)
> +nbctl_lport_get_macs(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_port *lport;
>      struct svec macs;
>      const char *mac;
>      size_t i;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, id);
> +    lport = lport_by_name_or_uuid(ctx, id);
>      if (!lport) {
>          return;
>      }
> @@ -575,19 +802,18 @@ nbctl_lport_get_macs(struct ovs_cmdl_context *ctx)
>      }
>      svec_sort(&macs);
>      SVEC_FOR_EACH(i, mac, &macs) {
> -        printf("%s\n", mac);
> +        ds_put_format(&ctx->output, "%s\n", mac);
>      }
>      svec_destroy(&macs);
>  }
>
>  static void
> -nbctl_lport_set_port_security(struct ovs_cmdl_context *ctx)
> +nbctl_lport_set_port_security(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_port *lport;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, id);
> +    lport = lport_by_name_or_uuid(ctx, id);
>      if (!lport) {
>          return;
>      }
> @@ -597,16 +823,15 @@ nbctl_lport_set_port_security(struct
> ovs_cmdl_context *ctx)
>  }
>
>  static void
> -nbctl_lport_get_port_security(struct ovs_cmdl_context *ctx)
> +nbctl_lport_get_port_security(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_port *lport;
>      struct svec addrs;
>      const char *addr;
>      size_t i;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, id);
> +    lport = lport_by_name_or_uuid(ctx, id);
>      if (!lport) {
>          return;
>      }
> @@ -617,35 +842,34 @@ nbctl_lport_get_port_security(struct
> ovs_cmdl_context *ctx)
>      }
>      svec_sort(&addrs);
>      SVEC_FOR_EACH(i, addr, &addrs) {
> -        printf("%s\n", addr);
> +        ds_put_format(&ctx->output, "%s\n", addr);
>      }
>      svec_destroy(&addrs);
>  }
>
>  static void
> -nbctl_lport_get_up(struct ovs_cmdl_context *ctx)
> +nbctl_lport_get_up(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_port *lport;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, id);
> +    lport = lport_by_name_or_uuid(ctx, id);
>      if (!lport) {
>          return;
>      }
>
> -    printf("%s\n", (lport->up && *lport->up) ? "up" : "down");
> +    ds_put_format(&ctx->output,
> +                  "%s\n", (lport->up && *lport->up) ? "up" : "down");
>  }
>
>  static void
> -nbctl_lport_set_enabled(struct ovs_cmdl_context *ctx)
> +nbctl_lport_set_enabled(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const char *state = ctx->argv[2];
>      const struct nbrec_logical_port *lport;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, id);
> +    lport = lport_by_name_or_uuid(ctx, id);
>      if (!lport) {
>          return;
>      }
> @@ -662,30 +886,28 @@ nbctl_lport_set_enabled(struct ovs_cmdl_context *ctx)
>  }
>
>  static void
> -nbctl_lport_get_enabled(struct ovs_cmdl_context *ctx)
> +nbctl_lport_get_enabled(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_port *lport;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, id);
> +    lport = lport_by_name_or_uuid(ctx, id);
>      if (!lport) {
>          return;
>      }
>
> -    printf("%s\n",
> -           (!lport->enabled || *lport->enabled) ? "enabled" : "disabled");
> +    ds_put_format(&ctx->output, "%s\n",
> +                  !lport->enabled || *lport->enabled ? "enabled" :
> "disabled");
>  }
>
>  static void
> -nbctl_lport_set_type(struct ovs_cmdl_context *ctx)
> +nbctl_lport_set_type(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const char *type = ctx->argv[2];
>      const struct nbrec_logical_port *lport;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, id);
> +    lport = lport_by_name_or_uuid(ctx, id);
>      if (!lport) {
>          return;
>      }
> @@ -694,30 +916,28 @@ nbctl_lport_set_type(struct ovs_cmdl_context *ctx)
>  }
>
>  static void
> -nbctl_lport_get_type(struct ovs_cmdl_context *ctx)
> +nbctl_lport_get_type(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_port *lport;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, id);
> +    lport = lport_by_name_or_uuid(ctx, id);
>      if (!lport) {
>          return;
>      }
>
> -    printf("%s\n", lport->type);
> +    ds_put_format(&ctx->output, "%s\n", lport->type);
>  }
>
>  static void
> -nbctl_lport_set_options(struct ovs_cmdl_context *ctx)
> +nbctl_lport_set_options(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_port *lport;
>      size_t i;
>      struct smap options = SMAP_INITIALIZER(&options);
>
> -    lport = lport_by_name_or_uuid(nb_ctx, id);
> +    lport = lport_by_name_or_uuid(ctx, id);
>      if (!lport) {
>          return;
>      }
> @@ -738,20 +958,19 @@ nbctl_lport_set_options(struct ovs_cmdl_context *ctx)
>  }
>
>  static void
> -nbctl_lport_get_options(struct ovs_cmdl_context *ctx)
> +nbctl_lport_get_options(struct ctl_context *ctx)
>  {
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *id = ctx->argv[1];
>      const struct nbrec_logical_port *lport;
>      struct smap_node *node;
>
> -    lport = lport_by_name_or_uuid(nb_ctx, id);
> +    lport = lport_by_name_or_uuid(ctx, id);
>      if (!lport) {
>          return;
>      }
>
>      SMAP_FOR_EACH(node, &lport->options) {
> -        printf("%s=%s\n", node->key, node->value);
> +        ds_put_format(&ctx->output, "%s=%s\n", node->key, node->value);
>      }
>  }
>
> @@ -793,14 +1012,13 @@ acl_cmp(const void *acl1_, const void *acl2_)
>  }
>
>  static void
> -nbctl_acl_list(struct ovs_cmdl_context *ctx)
> +nbctl_acl_list(struct ctl_context *ctx)
>  {
>      const struct nbrec_logical_switch *lswitch;
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const struct nbrec_acl **acls;
>      size_t i;
>
> -    lswitch = lswitch_by_name_or_uuid(nb_ctx, ctx->argv[1]);
> +    lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1]);
>      if (!lswitch) {
>          return;
>      }
> @@ -822,15 +1040,14 @@ nbctl_acl_list(struct ovs_cmdl_context *ctx)
>  }
>
>  static void
> -nbctl_acl_add(struct ovs_cmdl_context *ctx)
> +nbctl_acl_add(struct ctl_context *ctx)
>  {
>      const struct nbrec_logical_switch *lswitch;
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *action = ctx->argv[5];
>      const char *direction;
>      int64_t priority;
>
> -    lswitch = lswitch_by_name_or_uuid(nb_ctx, ctx->argv[1]);
> +    lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1]);
>      if (!lswitch) {
>          return;
>      }
> @@ -860,12 +1077,12 @@ nbctl_acl_add(struct ovs_cmdl_context *ctx)
>      }
>
>      /* Create the acl. */
> -    struct nbrec_acl *acl = nbrec_acl_insert(nb_ctx->txn);
> +    struct nbrec_acl *acl = nbrec_acl_insert(ctx->txn);
>      nbrec_acl_set_priority(acl, priority);
>      nbrec_acl_set_direction(acl, direction);
>      nbrec_acl_set_match(acl, ctx->argv[4]);
>      nbrec_acl_set_action(acl, action);
> -    if (ctx->argc == 7 && ctx->argv[6][0] == 'l') {
> +    if (shash_find(&ctx->options, "--log") != NULL) {
>          nbrec_acl_set_log(acl, true);
>      }
>
> @@ -880,14 +1097,13 @@ nbctl_acl_add(struct ovs_cmdl_context *ctx)
>  }
>
>  static void
> -nbctl_acl_del(struct ovs_cmdl_context *ctx)
> +nbctl_acl_del(struct ctl_context *ctx)
>  {
>      const struct nbrec_logical_switch *lswitch;
> -    struct nbctl_context *nb_ctx = ctx->pvt;
>      const char *direction;
>      int64_t priority = 0;
>
> -    lswitch = lswitch_by_name_or_uuid(nb_ctx, ctx->argv[1]);
> +    lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1]);
>      if (!lswitch) {
>          return;
>      }
> @@ -959,343 +1175,292 @@ nbctl_acl_del(struct ovs_cmdl_context *ctx)
>      }
>  }
>
> +static const struct ctl_table_class tables[] = {
> +    {&nbrec_table_logical_switch,
> +     {{&nbrec_table_logical_switch, &nbrec_logical_switch_col_name, NULL},
> +      {NULL, NULL, NULL}}},
> +
> +    {&nbrec_table_logical_port,
> +     {{&nbrec_table_logical_port, &nbrec_logical_port_col_name, NULL},
> +      {NULL, NULL, NULL}}},
> +
> +    {&nbrec_table_acl,
> +     {{NULL, NULL, NULL},
> +      {NULL, NULL, NULL}}},
> +
> +    {&nbrec_table_logical_router,
> +     {{&nbrec_table_logical_router, &nbrec_logical_router_col_name, NULL},
> +      {NULL, NULL, NULL}}},
> +
> +    {&nbrec_table_logical_router_port,
> +     {{&nbrec_table_logical_router_port,
> &nbrec_logical_router_port_col_name,
> +       NULL},
> +      {NULL, NULL, NULL}}},
> +
> +    {NULL, {{NULL, NULL, NULL}, {NULL, NULL, NULL}}}
> +};
> +
>  static void
> -parse_options(int argc, char *argv[])
> +run_prerequisites(struct ctl_command *commands, size_t n_commands,
> +                  struct ovsdb_idl *idl)
>  {
> -    enum {
> -        VLOG_OPTION_ENUMS,
> -    };
> -    static const struct option long_options[] = {
> -        {"db", required_argument, NULL, 'd'},
> -        {"help", no_argument, NULL, 'h'},
> -        {"options", no_argument, NULL, 'o'},
> -        {"version", no_argument, NULL, 'V'},
> -        VLOG_LONG_OPTIONS,
> -        STREAM_SSL_LONG_OPTIONS,
> -        {NULL, 0, NULL, 0},
> -    };
> -    char *short_options =
> ovs_cmdl_long_options_to_short_options(long_options);
> +    struct ctl_command *c;
>
> -    for (;;) {
> -        int c;
> -
> -        c = getopt_long(argc, argv, short_options, long_options, NULL);
> -        if (c == -1) {
> -            break;
> -        }
> +    for (c = commands; c < &commands[n_commands]; c++) {
> +        if (c->syntax->prerequisites) {
> +            struct ctl_context ctx;
>
> -        switch (c) {
> -        VLOG_OPTION_HANDLERS;
> -        STREAM_SSL_OPTION_HANDLERS;
> +            ds_init(&c->output);
> +            c->table = NULL;
>
> -        case 'd':
> -            db = optarg;
> -            break;
> +            ctl_context_init(&ctx, c, idl, NULL, NULL, NULL);
> +            (c->syntax->prerequisites)(&ctx);
> +            ctl_context_done(&ctx, c);
>
> -        case 'h':
> -            usage();
> -            exit(EXIT_SUCCESS);
> +            ovs_assert(!c->output.string);
> +            ovs_assert(!c->table);
> +        }
> +    }
> +}
>
> -        case 'o':
> -            ovs_cmdl_print_options(long_options);
> -            exit(EXIT_SUCCESS);
> +static void
> +do_nbctl(const char *args, struct ctl_command *commands, size_t
> n_commands,
> +         struct ovsdb_idl *idl)
> +{
> +    struct ovsdb_idl_txn *txn;
> +    enum ovsdb_idl_txn_status status;
> +    struct ovsdb_symbol_table *symtab;
> +    struct ctl_context ctx;
> +    struct ctl_command *c;
> +    struct shash_node *node;
> +    char *error = NULL;
> +
> +    txn = the_idl_txn = ovsdb_idl_txn_create(idl);
> +    if (dry_run) {
> +        ovsdb_idl_txn_set_dry_run(txn);
> +    }
> +
> +    ovsdb_idl_txn_add_comment(txn, "ovs-nbctl: %s", args);
> +
> +    symtab = ovsdb_symbol_table_create();
> +    for (c = commands; c < &commands[n_commands]; c++) {
> +        ds_init(&c->output);
> +        c->table = NULL;
> +    }
> +    ctl_context_init(&ctx, NULL, idl, txn, symtab, NULL);
> +    for (c = commands; c < &commands[n_commands]; c++) {
> +        ctl_context_init_command(&ctx, c);
> +        if (c->syntax->run) {
> +            (c->syntax->run)(&ctx);
> +        }
> +        ctl_context_done_command(&ctx, c);
>
> -        case 'V':
> -            ovs_print_version(0, 0);
> -            exit(EXIT_SUCCESS);
> +        if (ctx.try_again) {
> +            ctl_context_done(&ctx, NULL);
> +            goto try_again;
> +        }
> +    }
> +    ctl_context_done(&ctx, NULL);
>
> -        default:
> -            break;
> +    SHASH_FOR_EACH (node, &symtab->sh) {
> +        struct ovsdb_symbol *symbol = node->data;
> +        if (!symbol->created) {
> +            ctl_fatal("row id \"%s\" is referenced but never created
> (e.g. "
> +                      "with \"-- --id=%s create ...\")",
> +                      node->name, node->name);
> +        }
> +        if (!symbol->strong_ref) {
> +            if (!symbol->weak_ref) {
> +                VLOG_WARN("row id \"%s\" was created but no reference to
> it "
> +                          "was inserted, so it will not actually appear
> in "
> +                          "the database", node->name);
> +            } else {
> +                VLOG_WARN("row id \"%s\" was created but only a weak "
> +                          "reference to it was inserted, so it will not "
> +                          "actually appear in the database", node->name);
> +            }
>          }
>      }
>
> -    if (!db) {
> -        db = default_db();
> +    status = ovsdb_idl_txn_commit_block(txn);
> +    if (status == TXN_UNCHANGED || status == TXN_SUCCESS) {
> +        for (c = commands; c < &commands[n_commands]; c++) {
> +            if (c->syntax->postprocess) {
> +                ctl_context_init(&ctx, c, idl, txn, symtab, NULL);
> +                (c->syntax->postprocess)(&ctx);
> +                ctl_context_done(&ctx, c);
> +            }
> +        }
>      }
> +    error = xstrdup(ovsdb_idl_txn_get_error(txn));
>
> -    free(short_options);
> -}
> +    switch (status) {
> +    case TXN_UNCOMMITTED:
> +    case TXN_INCOMPLETE:
> +        OVS_NOT_REACHED();
>
> -static const struct ovs_cmdl_command all_commands[] = {
> -    {
> -        .name = "show",
> -        .usage = "[LSWITCH]",
> -        .min_args = 0,
> -        .max_args = 1,
> -        .handler = nbctl_show,
> -    },
> -    {
> -        .name = "lswitch-add",
> -        .usage = "[LSWITCH]",
> -        .min_args = 0,
> -        .max_args = 1,
> -        .handler = nbctl_lswitch_add,
> -    },
> -    {
> -        .name = "lswitch-del",
> -        .usage = "LSWITCH",
> -        .min_args = 1,
> -        .max_args = 1,
> -        .handler = nbctl_lswitch_del,
> -    },
> -    {
> -        .name = "lswitch-list",
> -        .usage = "",
> -        .min_args = 0,
> -        .max_args = 0,
> -        .handler = nbctl_lswitch_list,
> -    },
> -    {
> -        .name = "lswitch-set-external-id",
> -        .usage = "LSWITCH KEY [VALUE]",
> -        .min_args = 2,
> -        .max_args = 3,
> -        .handler = nbctl_lswitch_set_external_id,
> -    },
> -    {
> -        .name = "lswitch-get-external-id",
> -        .usage = "LSWITCH [KEY]",
> -        .min_args = 1,
> -        .max_args = 2,
> -        .handler = nbctl_lswitch_get_external_id,
> -    },
> -    {
> -        .name = "acl-add",
> -        .usage = "LSWITCH DIRECTION PRIORITY MATCH ACTION [log]",
> -        .min_args = 5,
> -        .max_args = 6,
> -        .handler = nbctl_acl_add,
> -    },
> -    {
> -        .name = "acl-del",
> -        .usage = "LSWITCH [DIRECTION [PRIORITY MATCH]]",
> -        .min_args = 1,
> -        .max_args = 4,
> -        .handler = nbctl_acl_del,
> -    },
> -    {
> -        .name = "acl-list",
> -        .usage = "LSWITCH",
> -        .min_args = 1,
> -        .max_args = 1,
> -        .handler = nbctl_acl_list,
> -    },
> -    {
> -        .name = "lport-add",
> -        .usage = "LSWITCH LPORT [PARENT] [TAG]",
> -        .min_args = 2,
> -        .max_args = 4,
> -        .handler = nbctl_lport_add,
> -    },
> -    {
> -        .name = "lport-del",
> -        .usage = "LPORT",
> -        .min_args = 1,
> -        .max_args = 1,
> -        .handler = nbctl_lport_del,
> -    },
> -    {
> -        .name = "lport-list",
> -        .usage = "LSWITCH",
> -        .min_args = 1,
> -        .max_args = 1,
> -        .handler = nbctl_lport_list,
> -    },
> -    {
> -        .name = "lport-get-parent",
> -        .usage = "LPORT",
> -        .min_args = 1,
> -        .max_args = 1,
> -        .handler = nbctl_lport_get_parent,
> -    },
> -    {
> -        .name = "lport-get-tag",
> -        .usage = "LPORT",
> -        .min_args = 1,
> -        .max_args = 1,
> -        .handler = nbctl_lport_get_tag,
> -    },
> -    {
> -        .name = "lport-set-external-id",
> -        .usage = "LPORT KEY [VALUE]",
> -        .min_args = 2,
> -        .max_args = 3,
> -        .handler = nbctl_lport_set_external_id,
> -    },
> -    {
> -        .name = "lport-get-external-id",
> -        .usage = "LPORT [KEY]",
> -        .min_args = 1,
> -        .max_args = 2,
> -        .handler = nbctl_lport_get_external_id,
> -    },
> -    {
> -        .name = "lport-set-macs",
> -        .usage = "LPORT [MAC]...",
> -        .min_args = 1,
> -        /* Accept however many arguments the system will allow. */
> -        .max_args = INT_MAX,
> -        .handler = nbctl_lport_set_macs,
> -    },
> -    {
> -        .name = "lport-get-macs",
> -        .usage = "LPORT",
> -        .min_args = 1,
> -        .max_args = 1,
> -        .handler = nbctl_lport_get_macs,
> -    },
> -    {
> -        .name = "lport-set-port-security",
> -        .usage = "LPORT [ADDRS]...",
> -        .min_args = 0,
> -        /* Accept however many arguments the system will allow. */
> -        .max_args = INT_MAX,
> -        .handler = nbctl_lport_set_port_security,
> -    },
> -    {
> -        .name = "lport-get-port-security",
> -        .usage = "LPORT",
> -        .min_args = 1,
> -        .max_args = 1,
> -        .handler = nbctl_lport_get_port_security,
> -    },
> -    {
> -        .name = "lport-get-up",
> -        .usage = "LPORT",
> -        .min_args = 1,
> -        .max_args = 1,
> -        .handler = nbctl_lport_get_up,
> -    },
> -    {
> -        .name = "lport-set-enabled",
> -        .usage = "LPORT STATE",
> -        .min_args = 2,
> -        .max_args = 2,
> -        .handler = nbctl_lport_set_enabled,
> -    },
> -    {
> -        .name = "lport-get-enabled",
> -        .usage = "LPORT",
> -        .min_args = 1,
> -        .max_args = 1,
> -        .handler = nbctl_lport_get_enabled,
> -    },
> -    {
> -        .name = "lport-set-type",
> -        .usage = "LPORT TYPE",
> -        .min_args = 2,
> -        .max_args = 2,
> -        .handler = nbctl_lport_set_type,
> -    },
> -    {
> -        .name = "lport-get-type",
> -        .usage = "LPORT",
> -        .min_args = 1,
> -        .max_args = 1,
> -        .handler = nbctl_lport_get_type,
> -    },
> -    {
> -        .name = "lport-set-options",
> -        .usage = "LPORT KEY=VALUE [KEY=VALUE]...",
> -        .min_args = 1,
> -        .max_args = INT_MAX,
> -        .handler = nbctl_lport_set_options
> -    },
> -    {
> -        .name = "lport-get-options",
> -        .usage = "LPORT",
> -        .min_args = 1,
> -        .max_args = 1,
> -        .handler = nbctl_lport_get_options,
> -    },
> -
> -    {
> -        /* sentinel */
> -        .name = NULL,
> -    },
> -};
> +    case TXN_ABORTED:
> +        /* Should not happen--we never call ovsdb_idl_txn_abort(). */
> +        ctl_fatal("transaction aborted");
>
> -static const struct ovs_cmdl_command *
> -get_all_commands(void)
> -{
> -    return all_commands;
> -}
> +    case TXN_UNCHANGED:
> +    case TXN_SUCCESS:
> +        break;
>
> -static const char *
> -default_db(void)
> -{
> -    static char *def;
> -    if (!def) {
> -        def = getenv("OVN_NB_DB");
> -        if (!def) {
> -            def = xasprintf("unix:%s/db.sock", ovs_rundir());
> -        }
> -    }
> -    return def;
> -}
> +    case TXN_TRY_AGAIN:
> +        goto try_again;
>
> -int
> -main(int argc, char *argv[])
> -{
> -    extern struct vlog_module VLM_reconnect;
> -    struct ovs_cmdl_context ctx;
> -    struct nbctl_context nb_ctx = { .idl = NULL, };
> -    enum ovsdb_idl_txn_status txn_status;
> -    unsigned int seqno;
> -    int res = 0;
> -    char *args;
> +    case TXN_ERROR:
> +        ctl_fatal("transaction error: %s", error);
>
> -    fatal_ignore_sigpipe();
> -    set_program_name(argv[0]);
> -    vlog_set_levels(NULL, VLF_CONSOLE, VLL_WARN);
> -    vlog_set_levels(&VLM_reconnect, VLF_ANY_DESTINATION, VLL_WARN);
> -    parse_options(argc, argv);
> -    nbrec_init();
> +    case TXN_NOT_LOCKED:
> +        /* Should not happen--we never call ovsdb_idl_set_lock(). */
> +        ctl_fatal("database not locked");
>
> -    args = process_escape_args(argv);
> +    default:
> +        OVS_NOT_REACHED();
> +    }
> +    free(error);
>
> -    nb_ctx.idl = ovsdb_idl_create(db, &nbrec_idl_class, true, false);
> -    ctx.pvt = &nb_ctx;
> -    ctx.argc = argc - optind;
> -    ctx.argv = argv + optind;
> +    ovsdb_symbol_table_destroy(symtab);
>
> -    seqno = ovsdb_idl_get_seqno(nb_ctx.idl);
> -    for (;;) {
> -        ovsdb_idl_run(nb_ctx.idl);
> +    for (c = commands; c < &commands[n_commands]; c++) {
> +        struct ds *ds = &c->output;
>
> -        if (!ovsdb_idl_is_alive(nb_ctx.idl)) {
> -            int retval = ovsdb_idl_get_last_error(nb_ctx.idl);
> -            VLOG_ERR("%s: database connection failed (%s)",
> -                    db, ovs_retval_to_string(retval));
> -            res = 1;
> -            break;
> -        }
> +        if (c->table) {
> +            table_print(c->table, &table_style);
> +        } else if (oneline) {
> +            size_t j;
>
> -        if (seqno != ovsdb_idl_get_seqno(nb_ctx.idl)) {
> -            nb_ctx.txn = ovsdb_idl_txn_create(nb_ctx.idl);
> -            ovsdb_idl_txn_add_comment(nb_ctx.txn, "ovn-nbctl: %s", args);
> -            ovs_cmdl_run_command(&ctx, get_all_commands());
> -            txn_status = ovsdb_idl_txn_commit_block(nb_ctx.txn);
> -            if (txn_status == TXN_TRY_AGAIN) {
> -                ovsdb_idl_txn_destroy(nb_ctx.txn);
> -                nb_ctx.txn = NULL;
> -                continue;
> -            } else {
> -                break;
> +            ds_chomp(ds, '\n');
> +            for (j = 0; j < ds->length; j++) {
> +                int ch = ds->string[j];
> +                switch (ch) {
> +                case '\n':
> +                    fputs("\\n", stdout);
> +                    break;
> +
> +                case '\\':
> +                    fputs("\\\\", stdout);
> +                    break;
> +
> +                default:
> +                    putchar(ch);
> +                }
>              }
> +            putchar('\n');
> +        } else {
> +            fputs(ds_cstr(ds), stdout);
>          }
> +        ds_destroy(&c->output);
> +        table_destroy(c->table);
> +        free(c->table);
>
> -        if (seqno == ovsdb_idl_get_seqno(nb_ctx.idl)) {
> -            ovsdb_idl_wait(nb_ctx.idl);
> -            poll_block();
> -        }
> +        shash_destroy_free_data(&c->options);
>      }
> +    free(commands);
> +    ovsdb_idl_txn_destroy(txn);
> +    ovsdb_idl_destroy(idl);
> +
> +    exit(EXIT_SUCCESS);
>
> -    if (nb_ctx.txn) {
> -        ovsdb_idl_txn_destroy(nb_ctx.txn);
> +try_again:
> +    /* Our transaction needs to be rerun, or a prerequisite was not met.
> Free
> +     * resources and return so that the caller can try again. */
> +    if (txn) {
> +        ovsdb_idl_txn_abort(txn);
> +        ovsdb_idl_txn_destroy(txn);
> +        the_idl_txn = NULL;
>      }
> -    ovsdb_idl_destroy(nb_ctx.idl);
> -    free(args);
> +    ovsdb_symbol_table_destroy(symtab);
> +    for (c = commands; c < &commands[n_commands]; c++) {
> +        ds_destroy(&c->output);
> +        table_destroy(c->table);
> +        free(c->table);
> +    }
> +    free(error);
> +}
> +
> +/* Frees the current transaction and the underlying IDL and then calls
> + * exit(status).
> + *
> + * Freeing the transaction and the IDL is not strictly necessary, but it
> makes
> + * for a clean memory leak report from valgrind in the normal case.  That
> makes
> + * it easier to notice real memory leaks. */
> +static void
> +nbctl_exit(int status)
> +{
> +    if (the_idl_txn) {
> +        ovsdb_idl_txn_abort(the_idl_txn);
> +        ovsdb_idl_txn_destroy(the_idl_txn);
> +    }
> +    ovsdb_idl_destroy(the_idl);
> +    exit(status);
> +}
> +
> +static const struct ctl_command_syntax nbctl_commands[] = {
> +    { "show", 0, 1, "[LSWITCH]", NULL, nbctl_show, NULL, "", RO },
> +
> +    /* lswitch commands. */
> +    { "lswitch-add", 0, 1, "[LSWITCH]", NULL, nbctl_lswitch_add,
> +      NULL, "", RW },
> +    { "lswitch-del", 1, 1, "LSWITCH", NULL, nbctl_lswitch_del,
> +      NULL, "", RW },
> +    { "lswitch-list", 0, 0, "", NULL, nbctl_lswitch_list, NULL, "", RO },
> +    { "lswitch-set-external-id", 2, 3, "LSWITCH KEY [VALUE]", NULL,
> +      nbctl_lswitch_set_external_id, NULL, "", RW },
> +    { "lswitch-get-external-id", 1, 2, "LSWITCH [KEY]", NULL,
> +      nbctl_lswitch_get_external_id, NULL, "", RO },
> +
> +    /* acl commands. */
> +    { "acl-add", 5, 5, "LSWITCH DIRECTION PRIORITY MATCH ACTION", NULL,
> +      nbctl_acl_add, NULL, "--log", RW },
> +    { "acl-del", 1, 4, "LSWITCH [DIRECTION [PRIORITY MATCH]]", NULL,
> +      nbctl_acl_del, NULL, "", RW },
> +    { "acl-list", 1, 1, "LSWITCH", NULL, nbctl_acl_list, NULL, "", RO },
> +
> +    /* lport commands. */
> +    { "lport-add", 2, 4, "LSWITCH LPORT [PARENT] [TAG]", NULL,
> nbctl_lport_add,
> +      NULL, "", RW },
> +    { "lport-del", 1, 1, "LPORT", NULL, nbctl_lport_del, NULL, "", RO },
> +    { "lport-list", 1, 1, "LSWITCH", NULL, nbctl_lport_list, NULL, "", RO
> },
> +    { "lport-get-parent", 1, 1, "LPORT", NULL, nbctl_lport_get_parent,
> NULL,
> +      "", RO },
> +    { "lport-get-tag", 1, 1, "LPORT", NULL, nbctl_lport_get_tag, NULL, "",
> +      RO },
> +    { "lport-set-external-id", 2, 3, "LPORT KEY [VALUE]", NULL,
> +      nbctl_lport_set_external_id, NULL, "", RW },
> +    { "lport-get-external-id", 1, 2, "LPORT [KEY]", NULL,
> +      nbctl_lport_get_external_id, NULL, "", RO },
> +    { "lport-set-macs", 1, INT_MAX, "LPORT [MAC]...", NULL,
> +      nbctl_lport_set_macs, NULL, "", RW },
> +    { "lport-get-macs", 1, 1, "LPORT", NULL, nbctl_lport_get_macs, NULL,
> +      "", RO },
> +    { "lport-set-port-security", 0, INT_MAX, "LPORT [ADDRS]...", NULL,
> +      nbctl_lport_set_port_security, NULL, "", RW },
> +    { "lport-get-port-security", 1, 1, "LPORT", NULL,
> +      nbctl_lport_get_port_security, NULL, "", RO },
> +    { "lport-get-up", 1, 1, "LPORT", NULL, nbctl_lport_get_up, NULL, "",
> RO },
> +    { "lport-set-enabled", 2, 2, "LPORT STATE", NULL,
> nbctl_lport_set_enabled,
> +      NULL, "", RW },
> +    { "lport-get-enabled", 1, 1, "LPORT", NULL, nbctl_lport_get_enabled,
> NULL,
> +      "", RO },
> +    { "lport-set-type", 2, 2, "LPORT TYPE", NULL, nbctl_lport_set_type,
> NULL,
> +      "", RW },
> +    { "lport-get-type", 1, 1, "LPORT", NULL, nbctl_lport_get_type, NULL,
> "",
> +      RO },
> +    { "lport-set-options", 1, INT_MAX, "LPORT KEY=VALUE [KEY=VALUE]...",
> NULL,
> +      nbctl_lport_set_options, NULL, "", RW },
> +    { "lport-get-options", 1, 1, "LPORT", NULL, nbctl_lport_get_options,
> NULL,
> +      "", RO },
> +
> +    {NULL, 0, 0, NULL, NULL, NULL, NULL, "", RO},
> +};
>
> -    exit(res);
> +/* Registers nbctl and common db commands. */
> +static void
> +nbctl_cmd_init(void)
> +{
> +    ctl_init(tables, NULL, nbctl_exit);
> +    ctl_register_commands(nbctl_commands);
>  }
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 200c703..d55732f 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -119,8 +119,8 @@ AT_SETUP([ovn-nbctl - ACLs])
>  OVN_NBCTL_TEST_START
>
>  AT_CHECK([ovn-nbctl lswitch-add ls0])
> -AT_CHECK([ovn-nbctl acl-add ls0 from-lport 600 udp drop log])
> -AT_CHECK([ovn-nbctl acl-add ls0 to-lport 500 udp drop log])
> +AT_CHECK([ovn-nbctl --log acl-add ls0 from-lport 600 udp drop])
> +AT_CHECK([ovn-nbctl --log acl-add ls0 to-lport 500 udp drop])
>  AT_CHECK([ovn-nbctl acl-add ls0 from-lport 400 tcp drop])
>  AT_CHECK([ovn-nbctl acl-add ls0 to-lport 300 tcp drop])
>  AT_CHECK([ovn-nbctl acl-add ls0 from-lport 200 ip drop])
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>

On Sun, Sep 13, 2015 at 09:57:58AM -0700, ALeX Wang wrote:
> On 11 September 2015 at 20:29, Ben Pfaff <blp@nicira.com> wrote:
> 
> > This makes ovn-nbctl into a pretty slavish imitation of ovn-nbctl, using
> > almost the same code.  It has two immediate benefits.  First, multiple
> > commands can now be chained together into a single ovn-nbctl invocation.
> > Second, the database commands such as "create", "set", and so on allow
> > queries and modifications that don't have specific commands already.
> > In the following commit, this allows testing the OVN ACL feature.
> >
> > Signed-off-by: Ben Pfaff <blp@nicira.com>

> imitation of ovn-sbctl?

Yes, oops

> one thing I noticed is that there is no implementation
> for 'struct nbctl_context' and 'cache'.  so, there is no
> guarantee that in a long cmd chain, the later cmd
> will never see the invalid db states.

I'm not sure what you mean.  The cache that ovs-vsctl implements is only
a cache; that is, leaving it out it doesn't affect correctness.  On the
other hand, if there is a cache, then logic is needed to invalidate the
cache if anything changes.

Maybe you could give an example of the problem you see?

Thanks,

Ben.

On 14 September 2015 at 08:33, Ben Pfaff <blp@nicira.com> wrote:

> On Sun, Sep 13, 2015 at 09:57:58AM -0700, ALeX Wang wrote:
> > On 11 September 2015 at 20:29, Ben Pfaff <blp@nicira.com> wrote:
> >
> > > This makes ovn-nbctl into a pretty slavish imitation of ovn-nbctl,
> using
> > > almost the same code.  It has two immediate benefits.  First, multiple
> > > commands can now be chained together into a single ovn-nbctl
> invocation.
> > > Second, the database commands such as "create", "set", and so on allow
> > > queries and modifications that don't have specific commands already.
> > > In the following commit, this allows testing the OVN ACL feature.
> > >
> > > Signed-off-by: Ben Pfaff <blp@nicira.com>
>
> > imitation of ovn-sbctl?
>
> Yes, oops
>
> > one thing I noticed is that there is no implementation
> > for 'struct nbctl_context' and 'cache'.  so, there is no
> > guarantee that in a long cmd chain, the later cmd
> > will never see the invalid db states.
>
> I'm not sure what you mean.  The cache that ovs-vsctl implements is only
> a cache; that is, leaving it out it doesn't affect correctness.  On the
> other hand, if there is a cache, then logic is needed to invalidate the
> cache if anything changes.
>
> Maybe you could give an example of the problem you see?
>
>

Sorry, I was confused to believe that cache is compulsory,... it is
just used for easy access, using hash.

Then, looks good to me~  maybe Russell would also like to look at this?

Thanks,
Alex Wang,




> Thanks,
>
> Ben.
>

On 09/14/2015 04:14 PM, ALeX Wang wrote:
> On 14 September 2015 at 08:33, Ben Pfaff <blp@nicira.com> wrote:
> 
>> On Sun, Sep 13, 2015 at 09:57:58AM -0700, ALeX Wang wrote:
>>> On 11 September 2015 at 20:29, Ben Pfaff <blp@nicira.com> wrote:
>>>
>>>> This makes ovn-nbctl into a pretty slavish imitation of ovn-nbctl,
>> using
>>>> almost the same code.  It has two immediate benefits.  First, multiple
>>>> commands can now be chained together into a single ovn-nbctl
>> invocation.
>>>> Second, the database commands such as "create", "set", and so on allow
>>>> queries and modifications that don't have specific commands already.
>>>> In the following commit, this allows testing the OVN ACL feature.
>>>>
>>>> Signed-off-by: Ben Pfaff <blp@nicira.com>
>>
>>> imitation of ovn-sbctl?
>>
>> Yes, oops
>>
>>> one thing I noticed is that there is no implementation
>>> for 'struct nbctl_context' and 'cache'.  so, there is no
>>> guarantee that in a long cmd chain, the later cmd
>>> will never see the invalid db states.
>>
>> I'm not sure what you mean.  The cache that ovs-vsctl implements is only
>> a cache; that is, leaving it out it doesn't affect correctness.  On the
>> other hand, if there is a cache, then logic is needed to invalidate the
>> cache if anything changes.
>>
>> Maybe you could give an example of the problem you see?
>>
>>
> 
> Sorry, I was confused to believe that cache is compulsory,... it is
> just used for easy access, using hash.
> 
> Then, looks good to me~  maybe Russell would also like to look at this?

Thanks for asking, but no need.  I think the idea of converting to
db-ctl-base makes perfect sense.  I'm happy if the code looks good to you.

I use it a good bit when testing things, so I'll be sure to report or
fix anything i run in to.  :-)

> On Sep 14, 2015, at 1:27 PM, Russell Bryant <rbryant@redhat.com> wrote:
> 
> On 09/14/2015 04:14 PM, ALeX Wang wrote:
>> On 14 September 2015 at 08:33, Ben Pfaff <blp@nicira.com> wrote:
>> 
>>> I'm not sure what you mean.  The cache that ovs-vsctl implements is only
>>> a cache; that is, leaving it out it doesn't affect correctness.  On the
>>> other hand, if there is a cache, then logic is needed to invalidate the
>>> cache if anything changes.
>>> 
>>> Maybe you could give an example of the problem you see?
>>> 
>>> 
>> 
>> Sorry, I was confused to believe that cache is compulsory,... it is
>> just used for easy access, using hash.
>> 
>> Then, looks good to me~  maybe Russell would also like to look at this?
> 
> Thanks for asking, but no need.  I think the idea of converting to
> db-ctl-base makes perfect sense.  I'm happy if the code looks good to you.
> 
> I use it a good bit when testing things, so I'll be sure to report or
> fix anything i run in to.  :-)

Ben, it doesn't look like you've checked this in.  Are you still waiting on an ACK?  Alex, did you do a full review?

--Justin

On 09/15/2015 01:22 PM, Justin Pettit wrote:
> 
>> On Sep 14, 2015, at 1:27 PM, Russell Bryant <rbryant@redhat.com> wrote:
>>
>> On 09/14/2015 04:14 PM, ALeX Wang wrote:
>>> On 14 September 2015 at 08:33, Ben Pfaff <blp@nicira.com> wrote:
>>>
>>>> I'm not sure what you mean.  The cache that ovs-vsctl implements is only
>>>> a cache; that is, leaving it out it doesn't affect correctness.  On the
>>>> other hand, if there is a cache, then logic is needed to invalidate the
>>>> cache if anything changes.
>>>>
>>>> Maybe you could give an example of the problem you see?
>>>>
>>>>
>>>
>>> Sorry, I was confused to believe that cache is compulsory,... it is
>>> just used for easy access, using hash.
>>>
>>> Then, looks good to me~  maybe Russell would also like to look at this?
>>
>> Thanks for asking, but no need.  I think the idea of converting to
>> db-ctl-base makes perfect sense.  I'm happy if the code looks good to you.
>>
>> I use it a good bit when testing things, so I'll be sure to report or
>> fix anything i run in to.  :-)
> 
> Ben, it doesn't look like you've checked this in.  Are you still
> waiting on an ACK?  Alex, did you do a full review?

I read Alex's response as he had done a full review, but I guess there
wasn't an ACK.  I'm happy to do a full review if it's still needed.

On Mon, Sep 14, 2015 at 01:14:43PM -0700, ALeX Wang wrote:
> On 14 September 2015 at 08:33, Ben Pfaff <blp@nicira.com> wrote:
> 
> > On Sun, Sep 13, 2015 at 09:57:58AM -0700, ALeX Wang wrote:
> > > On 11 September 2015 at 20:29, Ben Pfaff <blp@nicira.com> wrote:
> > >
> > > > This makes ovn-nbctl into a pretty slavish imitation of ovn-nbctl,
> > using
> > > > almost the same code.  It has two immediate benefits.  First, multiple
> > > > commands can now be chained together into a single ovn-nbctl
> > invocation.
> > > > Second, the database commands such as "create", "set", and so on allow
> > > > queries and modifications that don't have specific commands already.
> > > > In the following commit, this allows testing the OVN ACL feature.
> > > >
> > > > Signed-off-by: Ben Pfaff <blp@nicira.com>
> >
> > > imitation of ovn-sbctl?
> >
> > Yes, oops
> >
> > > one thing I noticed is that there is no implementation
> > > for 'struct nbctl_context' and 'cache'.  so, there is no
> > > guarantee that in a long cmd chain, the later cmd
> > > will never see the invalid db states.
> >
> > I'm not sure what you mean.  The cache that ovs-vsctl implements is only
> > a cache; that is, leaving it out it doesn't affect correctness.  On the
> > other hand, if there is a cache, then logic is needed to invalidate the
> > cache if anything changes.
> >
> > Maybe you could give an example of the problem you see?
> >
> >
> 
> Sorry, I was confused to believe that cache is compulsory,... it is
> just used for easy access, using hash.
> 
> Then, looks good to me~  maybe Russell would also like to look at this?

Thanks, applied to master.

On Tue, Sep 15, 2015 at 10:22:01AM -0700, Justin Pettit wrote:
> 
> > On Sep 14, 2015, at 1:27 PM, Russell Bryant <rbryant@redhat.com> wrote:
> > 
> > On 09/14/2015 04:14 PM, ALeX Wang wrote:
> >> On 14 September 2015 at 08:33, Ben Pfaff <blp@nicira.com> wrote:
> >> 
> >>> I'm not sure what you mean.  The cache that ovs-vsctl implements is only
> >>> a cache; that is, leaving it out it doesn't affect correctness.  On the
> >>> other hand, if there is a cache, then logic is needed to invalidate the
> >>> cache if anything changes.
> >>> 
> >>> Maybe you could give an example of the problem you see?
> >>> 
> >>> 
> >> 
> >> Sorry, I was confused to believe that cache is compulsory,... it is
> >> just used for easy access, using hash.
> >> 
> >> Then, looks good to me~  maybe Russell would also like to look at this?
> > 
> > Thanks for asking, but no need.  I think the idea of converting to
> > db-ctl-base makes perfect sense.  I'm happy if the code looks good to you.
> > 
> > I use it a good bit when testing things, so I'll be sure to report or
> > fix anything i run in to.  :-)
> 
> Ben, it doesn't look like you've checked this in.  Are you still
> waiting on an ACK?  Alex, did you do a full review?

I was just too busy with unrelated nonsense.  It's in now.