[ovs-dev] [PATCH] ovsdb-server: Write manager status information to Manager table.
blp at nicira.com
Tue Feb 1 18:38:41 UTC 2011
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:".
More information about the dev