On Mon, Jun 24, 2013 at 2:30 PM, Ben Pfaff <b...@nicira.com> wrote: > On Mon, Jun 24, 2013 at 01:39:15PM -0700, Gurucharan Shetty wrote: > > On Mon, Jun 24, 2013 at 1:10 PM, Ben Pfaff <b...@nicira.com> wrote: > > > > > 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 <gshe...@nicira.com> > > > > > 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? > > > > > I think I understand what you are saying. But consider the following > case. > > > > case 1: > > 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. > > 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. > > I don't mean to change the user-visible behavior from what you already > have here, only the implementation. > > Here's an example of what I mean. The new code tries to guess in > advance with ssl_path_valid() whether the next configuration will > succeed, and then it either does or doesn't use that in each of the > next runs based on that guess. The guess is supposed to be correct, > but it's always hard to tell whether that's exactly right. > > I'd suggest that, instead, one would modify reconfigure_from_db() and > the functions that it calls, as necessary, to return an error to the > caller if the configuration can't work. The initial > reconfigure_from_db() call at startup would abort with a fatal error > if reconfigure_from_db() failed. The ones during the main loop would > log an error (probably only each time the error string changes) > instead. > Right now, with a single database, we can provide a DB path without the db name. This patch would check for its validity when someone adds the second DB. I sent a V2 which makes the DB name mandatory and got rid of those checks here.
Also, for SSL paths, I moved it into reconfigure_from_db and only raise the error once. During startup, if the DB paths are invalid, we still exit (as before). Thanks, Guru
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
