[ovs-dev] [PATCH] ovsdb-server: Write manager status information to Manager table.
Andrew Evans
aevans at nicira.com
Tue Feb 1 18:30:53 UTC 2011
On 2/1/11 9:55 AM, Ben Pfaff wrote:
> On Mon, Jan 31, 2011 at 05:05:58PM -0800, Andrew Evans wrote:
> I think that parse_db_column() will segfault if its 'name_' argument
> does not contain a colon. Indeed:
Thanks for finding that. I'll fix it.
> 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.
> 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?
> I think that write_string_string_column() could use
> ovsdb_datum_sort_assert(), since its caller never adds a duplicate key.
Ok, will add that.
> 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?
-Andrew
More information about the dev
mailing list