[ovs-dev] [PATCH] ovsdb-server: Write manager status information to Manager table.
aevans at nicira.com
Tue Feb 1 00:46:50 UTC 2011
Thanks for your thorough review.
On 1/28/11 5:05 PM, Ben Pfaff wrote:
> I know that many of the files that you are modifying lack function-level
> comments, but it would still be nice to add a function comment on new
> functions, at least the ones that are not "static". In particular it's
> nice to know about ownership of allocated memory.
Agreed, will do. My preference is to put these in the header files,
since that's where I'd look if I'm going to call one of these functions,
but it seems the convention is to put them in the implementation, so
that's what I've done.
> ovsdb_jsonrpc_server_get_remote_status() xmallocs space for an shash but
> ovsdb_jsonrpc_server_destroy_remote_status() doesn't free that space,
> which looks like a leak to me. But it's more common in this source tree
> to make the caller provide a pointer to an shash, which usually turns
> out to be a local variable, so that the xmalloc() can be avoided
> entirely, but that's hardly a big deal either way.
> Also, I don't see anything doing shash_destroy() on the top-level shash
> created by ovsdb_jsonrpc_server_get_remote_status(). Because of the way
> shashes (and hmaps) work, this would only show up as a memory leak if
> there was more than one manager, so you might not have noticed using
> In ovsdb_jsonrpc_session_get_status(), it is an unusual design to have
> the different 'data' values of an shash point to data of different
> types. What motivates that design?
At first I was going to make it return a
> 'struct ovsdb_jsonrpc_remote_status' is never used (as you pointed out
but then decided to just return an shash since, ironically, that seemed
more conventional. But there's no getting away from the fact that it
holds heterogeneous data types. Turning "is_connected" from boolean to
string back to boolean again seemed silly.
I agree that collections should be homogenous, so I'll replace the inner
shash with 'struct ovsdb_jsonrpc_remote_status'.
> In ovsdb_jsonrpc_session_get_status(), there's no way that CONTAINER_OF
> can yield a null pointer.
Ok, null check removed.
> Similarly, jsonrpc_session_get_name() always
> returns nonnull as far as I can tell (maybe we should document that).
I guess that's true as long as the caller has called one of the
jsonrpc_session_open* functions, which seems like a reasonable
assumption. Ok, another null check gone.
> In main() in ovsdb-server.c, why is status_timer static?
Because it was static in bridge.c from whence I copied it. :)
> Also in
> main(), there should be something to wake up when the timer expires,
> e.g. poll_timer_wait_until(status_timer).
Ok, I've added a call to poll_timer_wait_until() just before
poll_block(). Is that the right thing to do?
> In get_datum(), it should be unnecessary to change the error
> message--ovsdb_type_to_english() fully describes the type.
The error message only printed the key type the caller requested. That
was fine when this code was only used to fetch scalar types, but now
that it's used to fetch maps, it needs to print the expected value type
and size as well, or the person reading the message sees nothing wrong.
> In read_column(), personally I'd find the "return" statement more
> readable as:
> return datum && datum->n ? datum->keys : NULL;
> In write_bool_column(), personally I'm not a fan of !! so I'd rather see
> the return statement written as "return datum != NULL;". Or the
> function could just be void, since nothing uses the return value.
Ok, the !! is gone, as are the return values from the write* functions,
since there's nothing more that can be done, really.
> In write_shash_column(), in the case where datum->n == n, nothing frees
> the old strings. You might be better off always constructing a new
> ovsdb_datum locally on the stack, then using ovsdb_datum_swap() to stick
> into the row, then freeing the local datum with ovsdb_datum_destroy().
> It could be more straightforward, and there would be no special cases.
Done. That looks more sane. BTW, I've renamed this function
'write_string_string_column' since it doesn't take an shash any more,
and the name really ought to describe the target column type anyway.
> In update_remote_status(), I don't see why the name of the table is
> hard-coded. The user provided the name of the table on the command line
> as part of --remote=db:..., so we should just honor that. It certainly
> shouldn't be a fatal runtime error for the table to not exist.
Ok, that was a major change, but I've revamped the code to walk the
shash of remote specs given on the command line, updating the row(s)
pointed to by each with connection status information.
> In update_remote_status(), I don't see any value in local variable
I'm trying to improve readability to those who don't have the meaning of
the arguments to ovsdb_txn_commit() memorized.
> The first issue that you warn about at least needs rate-limiting (e.g.
> VLOG_WARN_RL). The second one seems more of a bug than anything else,
> so it should have an assertion or just a segfault instead.
Those messages were redundant anyway, so they're gone.
> I'm really nervous about the way that ->data in the shash is managed.
> It's transferred to an ovsdb_datum by write_shash_column(), but if that
> doesn't happen nothing frees it as far as I can tell.
Hopefully that concern went away with the switch from shash to struct
> In vswitch.xml, time_in_state is documented as a time in seconds but I'm
> pretty sure that milliseconds are actually reported.
You're right. Changed.
> In vswitch.xml, it would be nice to document that "status" isn't carved
> in stone and will evolve over time, perhaps in incompatible ways.
Ok, I added some verbiage which I think conveys this.
> In vswitch.xml, I would say that time_in_state was the amount of time
> since 'state' changed.
> It would also be nice to informally document the meaning of each state.
Complete repost of patch forthcoming.
More information about the dev