<br><br><div class="gmail_quote">On Mon, Jun 24, 2013 at 1:10 PM, Ben Pfaff <span dir="ltr"><<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Thu, Jun 20, 2013 at 07:38:48AM -0700, Gurucharan Shetty wrote:<br>
> The commit allows a user to add a database file to a<br>
> ovsdb-server during run time. One can also remove a<br>
> database file from ovsdb-server's control.<br>
><br>
> Feature #14595.<br>
> Signed-off-by: Gurucharan Shetty <<a href="mailto:gshetty@nicira.com">gshetty@nicira.com</a>><br>
<br>
</div>The documentation could use a bit more detail. I think that readers<br>
might not know where to look, otherwise. How about this:<br>
<br>
----------------------------------------------------------------------<br>
<div class="im">.IP "\fBovsdb\-server/add\-db \fIdatabase\fR"<br>
</div>Adds \fIdatabase\fR (a file name) to the running<br>
\fBovsdb\-server\fR. The database file must already have been created<br>
and initialized using, for example, \fBovsdb\-tool create\fR.<br>
.<br>
<div class="im">.IP "\fBovsdb\-server/remove\-db \fIdatabase\fR"<br>
</div>Removes \fIdatabase\fR from the running \fBovsdb\-server\fR.<br>
\fIdatabase\fR must be a database name as listed by<br>
\fBovsdb-server/list\-dbs\fR.<br>
.IP<br>
If a remote has been configured that points to the specified<br>
\fIdatabase\fR (e.g. \fB\-\-remote=db:\fIdatabase\fB,\fR... on the<br>
command line), then it is automatically removed.<br>
.IP<br>
Any public key infrastructure options specified through this database<br>
(e.g. \fB\-\-private\-key=db:\fIdatabase,\fR... on the command line)<br>
are no longer checked for new changes, but any files previously<br>
configured are still used. (Adding \fIdatabase\fR back will cause the<br>
public key infrastructure options specified through it to update<br>
again.)<br>
.<br>
.IP "\fBovsdb\-server/list\-dbs"<br>
<div class="im">Outputs a list of the currently configured databases added either through<br>
</div><div class="im">the command line or through the \fBovsdb\-server/add\-db\fR command.<br></div></blockquote><div>Okay.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
</div>----------------------------------------------------------------------<br>
<br>
This commit seems to use the high-level approach of "figure out<br>
whether something will succeed, then actually do it only if it will<br>
succeed," when the thing that it actually wants to do will exit with a<br>
fatal error if it fails. I think that it would be safer to instead<br>
modify the code that exits with a fatal error to return an error to<br>
the caller to handle. With this approach, there is no chance that the<br>
code to figure out whether the action will succeed will be wrong, and<br>
as the code evolves later the two pieces of code cannot get out of<br>
sync. What do you think?<br></blockquote><div>I think I understand what you are saying. But consider the following case.</div><div><br></div><div>case 1:</div><div>We start with just one database. And we add a remote that points to a DB column without a schema. ex: Open_vSwitch,manager_options.</div>
<div>When I add a new database during runtime, the above remote is invalid(because of a missing schema). If I go ahead and add the database, and change the code such that in the main loop, I throw an error, then it will only go to the log files and there will be one error during every run. Do you feel this is more preferable? When a second database is added, say through a script, they won't get any error. Will we be okay if the main Open_vSwitch database stops working because of it?</div>
<div><br></div><div>Same with SSL. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The return value for ovsdb_server_remove_db() seems odd. I would make<br>
it return a bool.<br></blockquote><div>Okay. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
This change adds some lines ending in \ to<br>
ovsdb_server_add_database(). That's odd.<br></blockquote><div>I removed it. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
ssl_path_valid() has a function call wrapped across lines, that<br>
doesn't need to be.<br></blockquote><div>Okay. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
<br>
Ben.<br>
</blockquote></div><br>