[ovs-dev] [PATCH] ovsdb-server: Write manager status information to Manager table.

[Ben Pfaff]

On Tue, Feb 01, 2011 at 10:30:53AM -0800, Andrew Evans wrote:
> On 2/1/11 9:55 AM, Ben Pfaff wrote:
> > On Mon, Jan 31, 2011 at 05:05:58PM -0800, Andrew Evans wrote:
> > I'm not sure why you removed the warning in add_manager_options().  Did
> > it just get logged too much?  You could use VLOG_INFO_ONCE instead.
> 
> get_datum() already reports that, so it's redundant.

It does, but it only reports it at "debug" level, so the casual system
administrator won't quickly notice if he makes, e.g., a spelling error
in the --remote argument.  And I doubt that we want to increase the log
level in get_datum().

> > What's the reason for doing the update in two phases, first collecting
> > rows in get_remote_rows() and then updating them in the caller?  Does
> > this somehow work better than just iterating over the rows and modifying
> > them in a single pass?  It looks like this way actually involves more
> > code and more work, so I'm curious about the design motivation here.
> 
> I like that it tucks the type-checking grottiness away from
> update_remote_status(), keeping the latter clearer. But it does allocate
> memory, which wouldn't be necessary if it were inline. I'll rework this
> to just return the rows and move the HMAP_FOR_EACH into
> update_remote_status(). Sound ok?

It would look better to me.

> > In update_remote_status(), I'm not sure that it's guaranteed that
> > 'status' will be nonnull.  Are you sure about that?
> 
> I don't see how a target could get into the remotes hash yet not into
> the struct ovsdb_jsonrpc_server, which is what would have to happen for
> 'status' to be non-null. Do you?

It looks to me that this could happen if the target specified an
unsupported or misspelled protocol, e.g. "ssk:" instead of "ssl:".