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

[Ben Pfaff]

On Tue, Dec 22, 2009 at 10:42:36PM -0800, Justin Pettit wrote:
> 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.

An svec isn't a very good choice in many cases, because there's no
efficient way to both search and update an svec.  But an shash has no
such issue, so I've started using an shash in places where such
efficiency is desired.  It might make sense to introduce an "sset" or
some such but it doesn't yet seem important enough to me.

For now, I'll add a comment.

> > 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"?

It's a TCP server but a database client.

I changed that sentence to read:

    It supports JSON-RPC client connections over active or passive
    TCP/IP or Unix domain sockets.

I'm a little concerned that "active" and "passive" is a bit too much
jargon for the average reader, but at least it's unambiguous now.

> > +    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.

Yes, thank you for finding that.  I fixed all of the calls.

> > +    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".

Thanks, fixed.

> > -           "  --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?

Sure, I updated the wording.

> > # 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.

That's just an oversight.  I fixed it.  Thanks!

> Otherwise, it looks great.  This is much needed!

I'll push this soon, thanks again.