[ovs-dev] [PATCH 2/2] ovsdb-server: Add and remove databases during run time.

Ben Pfaff blp at nicira.com
Mon Jun 24 20:10:36 UTC 2013


On Thu, Jun 20, 2013 at 07:38:48AM -0700, Gurucharan Shetty wrote:
> The commit allows a user to add a database file to a
> ovsdb-server during run time. One can also remove a
> database file from ovsdb-server's control.
> 
> Feature #14595.
> Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>

The documentation could use a bit more detail.  I think that readers
might not know where to look, otherwise.  How about this:

----------------------------------------------------------------------
.IP "\fBovsdb\-server/add\-db \fIdatabase\fR"
Adds \fIdatabase\fR (a file name) to the running
\fBovsdb\-server\fR. The database file must already have been created
and initialized using, for example, \fBovsdb\-tool create\fR.
.
.IP "\fBovsdb\-server/remove\-db \fIdatabase\fR"
Removes \fIdatabase\fR from the running \fBovsdb\-server\fR.
\fIdatabase\fR must be a database name as listed by
\fBovsdb-server/list\-dbs\fR.
.IP
If a remote has been configured that points to the specified
\fIdatabase\fR (e.g. \fB\-\-remote=db:\fIdatabase\fB,\fR... on the
command line), then it is automatically removed.
.IP
Any public key infrastructure options specified through this database
(e.g. \fB\-\-private\-key=db:\fIdatabase,\fR... on the command line)
are no longer checked for new changes, but any files previously
configured are still used. (Adding \fIdatabase\fR back will cause the
public key infrastructure options specified through it to update
again.)
.
.IP "\fBovsdb\-server/list\-dbs"
Outputs a list of the currently configured databases added either through
the command line or through the \fBovsdb\-server/add\-db\fR command.
----------------------------------------------------------------------

This commit seems to use the high-level approach of "figure out
whether something will succeed, then actually do it only if it will
succeed," when the thing that it actually wants to do will exit with a
fatal error if it fails.  I think that it would be safer to instead
modify the code that exits with a fatal error to return an error to
the caller to handle.  With this approach, there is no chance that the
code to figure out whether the action will succeed will be wrong, and
as the code evolves later the two pieces of code cannot get out of
sync.  What do you think?

The return value for ovsdb_server_remove_db() seems odd.  I would make
it return a bool.

This change adds some lines ending in \ to
ovsdb_server_add_database().  That's odd.

ssl_path_valid() has a function call wrapped across lines, that
doesn't need to be.

Thanks,

Ben.



More information about the dev mailing list