On Mon, Apr 06, 2015 at 03:14:59PM -0700, Justin Pettit wrote:
> Add new ovn-controller daemon that runs locally on transport nodes.
> This initial version registers itself in the Chassis table and registers
> logical ports to the appropriate rows in the Bindings table.
>
> Signed-off-by: Justin Pettit <jpet...@nicira.com>
Thanks for sending this out! Here's a really quick review since I'm
short on time.
"git am" says:
/home/blp/ovs/.git/rebase-apply/patch:834: trailing whitespace.
* since we'll need to get the OVN OVSDB remote. */
warning: 1 line adds whitespace errors.
"sparse" says:
../ovn/ovn-controller.c:54:6: warning: symbol 'chassis_name' was not
declared. Should it be static?
which probably means that it should be static.
Maybe it's time to make subdirectories for daemons as you suggested
earlier. It's going to start getting confusing which file goes with
which daemon and which ones are shared.
bindings.c
----------
#include "bindings.h" should go just after #include <config.h>, see
CodingStyle.
bindings_init() should drop OVS_UNUSED since it does use its ovn_idl
parameter. Ditto for bindings_run().
I'm confused about why mod_binding() does a whole round-trip to the
database server to make a single change. Why doesn't the code batch
all of its changes into a larger transaction?
In bindings_run(), sset seems like a more natural fit for 'lports',
since it's a set of strings and that seems to be what we need here.
In bindings_run(), it would be more efficient to write
if (strlen(bindings_rec->chassis)) {
as:
if (bindings_rec->chassis[0]) {
In bindings_run(), I think it's risky to loop over the rows in the
bindings table while we add and delete rows in it. The tables are
just hash tables so the usual caveats apply. Same in
bindings_destroy().
chassis.c
---------
register_chassis() does an xmalloc() and never frees the data.
ovnrec_chassis_set_encaps() doesn't take ownership, so I don't think
the xmalloc() is needed (given that n_encaps is fixed at 1)--use a
local instead?
chassis_run() seems to assume that there's always an Open_vSwitch
record. That's the usual case but we don't want to segfault if the
database hasn't been properly initialized yet.
ovn-controller.c
----------------
Same comment as above for assuming there always an Open_vSwitch
record, in get_core_config().
In parse_options(), I think I'd use OPF13_VERSION because that's the
OpenFlow version I'd prefer to start with.
In parse_options(), the fatal error "exactly one non-option argument
required; " is inaccurate (we accept 0 or 1 arguments).
ovn-controller.8.xml
--------------------
I don't see anything doing @-substitution on this file, so I see
things like @RUNDIR@ in the installed manpage.
Synopsys is an EDA company, "synopsis" is the English word:
+ <h1>Synopsys</h1>
It's a little weird that OVS-DATABASE is capitalized in the manpage.
I think the usual style would be to make it lowercase but italicized
(with <var>).
It's a little weird to list the options without explanations like the
rest of the OVS manpages have.
I think you'll get better formatting for related options if you use
<dl> instead of <p>, e.g.:
<dl>
<dt><code>-v</code><var>spec</var>,
<code>--verbose=</code><var>spec</var></dt>
<dt><code>-v</code>, <code>--verbose</code></dt>
<dd>
Explanation...
</dd>
<dt><code>--log-file</code>[<code>=</code><var>file</var>]</t>
<dd>
Explanation...
</dd>
<dt><code>--syslog-target=</code><var>host</var><code>:</code><var>port</var></dt>
<dd>
Explanation...
</dd>
</dl>
Are all of the external_ids mandatory or can we pick good defaults for
some of them?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
