[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