> On Apr 6, 2015, at 4:00 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> 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.
Argh, I set up a script to catch these, but apparently this got committed
before that went into effect. Fixed.
> "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.
It was shared between all the files. However, I changed the code to pass
around a context, which is probably cleaner.
> 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.
Done.
> bindings.c
> ----------
>
> #include "bindings.h" should go just after #include <config.h>, see
> CodingStyle.
Done.
> bindings_init() should drop OVS_UNUSED since it does use its ovn_idl
> parameter. Ditto for bindings_run().
Whoops. That was a leftover from an earlier version.
> 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?
I was trying to make the comments helpful in the log, but they'll likely get
overwhelming at scale, so the new version batches them into a single
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.
Good catch. That was from an earlier version that did things differently.
> In bindings_run(), it would be more efficient to write
> if (strlen(bindings_rec->chassis)) {
> as:
> if (bindings_rec->chassis[0]) {
Done.
> 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().
I'm not sure I see the problem with bindings_run(). I would think the
transaction would fail because it's dirty, and then it would get executed
again. But, I'm probably missing something. I do see an issue with
bindings_destroy(), which I think I resolved. Please take another look at v2,
though.
> 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?
Yeah, that was a failed attempt at future-proofing. Fixed.
> 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.
Done.
> ovn-controller.c
> ----------------
>
> Same comment as above for assuming there always an Open_vSwitch
> record, in get_core_config().
Done.
> In parse_options(), I think I'd use OPF13_VERSION because that's the
> OpenFlow version I'd prefer to start with.
Good point. Done.
> In parse_options(), the fatal error "exactly one non-option argument
> required; " is inaccurate (we accept 0 or 1 arguments).
Done.
> ovn-controller.8.xml
> --------------------
>
> I don't see anything doing @-substitution on this file, so I see
> things like @RUNDIR@ in the installed manpage.
I was going to leave it as a reminder to rework the XML man page generation,
but it does look weird, so I took it out.
> Synopsys is an EDA company, "synopsis" is the English word:
> + <h1>Synopsys</h1>
Fixed.
> 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>).
Fixed.
> It's a little weird to list the options without explanations like the
> rest of the OVS manpages have.
I just copied that out of the ovn-nbctl man page. I agree they should be
improved, though. Along with the %RUNDIR% substitution, I'd like to be able to
include other man page fragments like the rest of OVS. I just dropped them for
now, since they really should be shared between the different daemons.
> 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>
I'll do that when I reintroduce them. Thanks.
> Are all of the external_ids mandatory or can we pick good defaults for
> some of them?
I've made "br-int" the default for the integration bridge. I didn't think the
others have good default. We should think about what the default encapsulation
type is, but that has impact on the tunnel key length, which we haven't decided
how to handle yet.
Thanks for the quick review. I'm sending out v2 now.
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
