[ovs-dev] [PATCH 4/4] ovsdb-server: Make database connections configurable from database itself.

[Justin Pettit]

On Dec 18, 2009, at 1:47 PM, Ben Pfaff wrote:

> void
> -ovsdb_jsonrpc_server_listen(struct ovsdb_jsonrpc_server *svr,
> -                            struct pstream *pstream)
> +ovsdb_jsonrpc_server_set_remotes(struct ovsdb_jsonrpc_server *svr,
> +                                 const struct shash *new_remotes)

Is there a reason that "new_remotes" is a shash instead of a svec?  I don't think the data portion is used and may be confusing to gentleman callers.

> The \fBovsdb\-server\fR program provides RPC interfaces to an Open
> vSwitch database (OVSDB).  It can listen for JSON-RPC connections from
> -TCP/IP or Unix domain socket clients (with \fB\-\-listen\fR), connect to
> -remote JSON-RPC TCP/IP or Unix domain socket clients (with
> -\fB\-\-connect\fR).
> +TCP/IP or Unix domain socket clients and connect to remote JSON-RPC
> +TCP/IP or Unix domain socket clients.

Shouldn't that final "clients" be "servers"?

> +    name = xstrdup(name_);
> +    db_prefix = strtok_r(name, ":", &save_ptr);
> +    table_name = strtok_r(NULL, ",", &save_ptr);
> +    column_name = strtok_r(NULL, ",", &save_ptr);
> +    if (!table_name || !column_name) {
> +        ovs_fatal(0, "remote \"%s\": invalid syntax", name);
> +    }

In this call to ovs_fatal, did you actually want to print "name_" instead of "name"?  I think "name" will be mangled by the strtok_r calls.  There are a few other ovs_fatal calls further down that also use "name" as an argument.

> +    if (column->type.key_type != OVSDB_TYPE_STRING
> +        || column->type.value_type != OVSDB_TYPE_VOID) {
> +        ovs_fatal(0, "remote \"%s\": type of table \"%s\" column \"%s\" is "
> +                  "not string or set of string",

I'm guessing OVSDB_TYPE_VOID is as close as we can get to type-checking a set of strings.  Regardless, that error should probably read "set of strings".

> -           "  --connect=REMOTE        make active connection to REMOTE\n"
> -           "  --listen=LOCAL          passively listen on LOCAL\n");
> +           "  --remote=REMOTE         connect to REMOTE\n");

I find "--remote" a bit confusing, since it's used for both connecting and listening, but I don't have a better suggestion for a name.  Would we be able to at least modify the description to read "connect or listen to REMOTE" or something similar?

> # Config variables specific to ovsdb-server
> -: ${OVSDB_SERVER_LISTEN:=punix:/var/run/ovsdb-server}
> -: ${OVSDB_SERVER_CONNECT:=}
> +: ${OVSDB_SERVER_REMOTES:=punix:/var/run/ovsdb-server}

Is there a reason "db:Open_vSwitch,managers" isn't included in OVSDB_SERVER_REMOTES  like in the sysconfig template?  It seems like a useful default, and we've generally followed the convention that all the items in the template are commented out, but match the default.

Otherwise, it looks great.  This is much needed!

--Justin