Thx a lot Russel, for the review,~ On Mon, Jul 6, 2015 at 11:02 AM, Russell Bryant <rbry...@redhat.com> wrote:
> On 07/05/2015 01:38 AM, Alex Wang wrote: > > This commit adds ovn-sbctl to ovn family by using the db-ctl-base > > library. > > > > Signed-off-by: Alex Wang <al...@nicira.com> > > Acked-by: Ben Pfaff <b...@nicira.com> > > > > --- > > PATCH->V2: > > - change command add/del-ch to add/del-chassis. > > - refine the manual based on comments from Ben. > > --- > > manpages.mk | 12 + > > ovn/.gitignore | 2 + > > ovn/automake.mk | 9 + > > ovn/ovn-sbctl.8.in | 160 ++++++++++ > > ovn/ovn-sbctl.c | 837 > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > tests/automake.mk | 5 +- > > tests/ovn-sbctl.at | 61 ++++ > > tests/testsuite.at | 1 + > > 8 files changed, 1085 insertions(+), 2 deletions(-) > > create mode 100644 ovn/ovn-sbctl.8.in > > create mode 100644 ovn/ovn-sbctl.c > > create mode 100644 tests/ovn-sbctl.at > > > > diff --git a/manpages.mk b/manpages.mk > > index 3cec260..032cb26 100644 > > --- a/manpages.mk > > +++ b/manpages.mk > > @@ -1,5 +1,17 @@ > > # Generated automatically -- do not modify! -*- buffer-read-only: t > -*- > > > > +ovn/ovn-sbctl.8: \ > > + ovn/ovn-sbctl.8.in \ > > + lib/db-ctl-base.man \ > > + lib/table.man \ > > + ovsdb/remote-active.man \ > > + ovsdb/remote-passive.man > > +ovn/ovn-sbctl.8.in: > > +lib/db-ctl-base.man: > > +lib/table.man: > > +ovsdb/remote-active.man: > > +ovsdb/remote-passive.man: > > + > > ovsdb/ovsdb-client.1: \ > > ovsdb/ovsdb-client.1.in \ > > lib/common-syn.man \ > > diff --git a/ovn/.gitignore b/ovn/.gitignore > > index 4c13616..2d4835a 100644 > > --- a/ovn/.gitignore > > +++ b/ovn/.gitignore > > @@ -7,3 +7,5 @@ > > /ovn-sb.pic > > /ovn-nbctl > > /ovn-nbctl.8 > > +/ovn-sbctl > > +/ovn-sbctl.8 > > This isn't really related to this patch, but maybe we should create a > ovn/utilities/ directory for ovn-nbctl, ovn-sbctl, and whatever else we > come up with. > Yeah, makes sense, > > diff --git a/ovn/ovn-sbctl.8.in b/ovn/ovn-sbctl.8.in > > new file mode 100644 > > index 0000000..1cf9b7d > > --- /dev/null > > +++ b/ovn/ovn-sbctl.8.in > > @@ -0,0 +1,160 @@ > > +.\" -*- nroff -*- > > How about doing this in the XML format? It seems much easier to read > and edit that way. > This is to take advantage of the lib/db-ctl-base.man. Maybe I should convert all ovs-vsctl, vtep-ctl, ovs-sbctl manpage to xml format. > > new file mode 100644 > > index 0000000..8c82d03 > > --- /dev/null > > +++ b/ovn/ovn-sbctl.c > > @@ -0,0 +1,837 @@ > > +/* > > + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc. > > Presumably just 2015? > > Though these days I personally prefer not including this line at all > since it's not necessary, and is rarely accurate in a collaborative open > source project. Keeping it is consistent with the rest of OVS, though ... > > > + * > Sorry for the careless copy/past, will fix it, and be more careful, > > + * Licensed under the Apache License, Version 2.0 (the "License"); > > + * you may not use this file except in compliance with the License. > > + * You may obtain a copy of the License at: > > + * > > + * http://www.apache.org/licenses/LICENSE-2.0 > > + * > > + * Unless required by applicable law or agreed to in writing, software > > + * distributed under the License is distributed on an "AS IS" BASIS, > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > implied. > > + * See the License for the specific language governing permissions and > > + * limitations under the License. > > + */ > > + > > +#include <config.h> > > + > > +#include <ctype.h> > > +#include <errno.h> > > +#include <float.h> > > +#include <getopt.h> > > +#include <inttypes.h> > > +#include <signal.h> > > +#include <stdarg.h> > > +#include <stdlib.h> > > +#include <string.h> > > +#include <unistd.h> > > + > > +#include "db-ctl-base.h" > > + > > +#include "command-line.h" > > +#include "compiler.h" > > +#include "dynamic-string.h" > > +#include "fatal-signal.h" > > +#include "json.h" > > +#include "ovsdb-data.h" > > +#include "ovsdb-idl.h" > > +#include "poll-loop.h" > > +#include "process.h" > > +#include "sset.h" > > +#include "shash.h" > > +#include "ovn/lib/ovn-sb-idl.h" > > +#include "table.h" > > +#include "timeval.h" > > +#include "util.h" > > +#include "openvswitch/vlog.h" > > + > > +VLOG_DEFINE_THIS_MODULE(sbctl); > > + > > +struct sbctl_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; > > + > > +static void sbctl_cmd_init(void); > > +OVS_NO_RETURN static void usage(void); > > +static void parse_options(int argc, char *argv[], struct shash > *local_options); > > +static void run_prerequisites(struct ctl_command[], size_t n_commands, > > + struct ovsdb_idl *); > > +static void do_sbctl(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 ctl_command *commands; > > + struct shash local_options; > > + unsigned int seqno; > > + size_t n_commands; > > + char *args; > > + > > + 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); > > + sbrec_init(); > > + > > + sbctl_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, &sbrec_idl_class, false, > 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_sbctl(args, commands, n_commands, idl); > > + } > > + > > + if (seqno == ovsdb_idl_get_seqno(idl)) { > > + ovsdb_idl_wait(idl); > > + poll_block(); > > + } > > + } > > +} > > + > > +static void > > +parse_options(int argc, char *argv[], struct shash *local_options) > > +{ > > + enum { > > + OPT_DB = UCHAR_MAX + 1, > > + OPT_ONELINE, > > + OPT_NO_SYSLOG, > > + OPT_DRY_RUN, > > + OPT_PEER_CA_CERT, > > + 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, > > + 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_sbctl, 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(); > > + > > + 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", sbrec_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) > > + > > + case '?': > > + exit(EXIT_FAILURE); > > + > > + default: > > + abort(); > > + } > > + } > > + free(short_options); > > + > > + if (!db) { > > + db = ctl_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) > > +{ > > + printf("\ > > +%s: ovs-vswitchd management utility\n\ > > +usage: %s [OPTIONS] COMMAND [ARG...]\n\ > > +\n\ > > +SouthBound DB commands:\n\ > > + show print overview of database contents\n\ > > +\n\ > > +Chassis commands:\n\ > > + add-chassis CHASSIS create a new chassis named CHASSIS\n\ > > + del-chassis CHASSIS delete CHASSIS and all of its encaps,\n\ > > + and gateway_ports\n\ > > +\n\ > > +Binding commands:\n\ > > + bind-lport LPORT CHASSIS bind logical port LPORT to CHASSIS\n\ > > + unbind-lport LPORT delete the binding of logical port > LPORT\n\ > > I'm trying to think of when any of these commands would be used. > add-chassis and bind-lport are always done by ovn-controller. I could > see del-chassis needed in some cases where ovn-controller didn't exit > cleanly and an administrator is trying to do some cleanup. I suppose > unbind-port could be used in cleanup, too. > > Are these commands just for test automation with ovn-controller out of > the picture? If so, maybe the help output could make it clear that the > commands should never be needed? > > I'm just asking because I want to avoid making OVN confusing by exposing > things to administrators that should never really be used. > Yeah, these are only for testing purpose. I'll make it clear in usage() !~ > -- > Russell Bryant > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
