[ovs-dev] [lacp 07/10] vswitchd: Modularize LACP.

[Ben Pfaff]

On Tue, Mar 15, 2011 at 04:03:20PM -0700, Ethan Jackson wrote:
> This commit pulls LACP code out of the bridge into its own LACP
> module.  Currently this module is only used by the existing bonding
> infrastructure.

Thanks.  Making this more modular makes the code easier to understand.

I didn't see any obvious changes in behavior due to this.  (That means
that the only bugs are subtle ones!)

A comment in struct port has NULL written as NULl (lowercase final
'l').

It might be worthwhile to make lacp_slave_may_enable() accept a NULL
'lacp' argument and always return 'true'.  Or maybe you don't like
that, *shrug*.

The lacp_active member in struct port seems like a bit of a oddity.
It seems to me that it could be fully integrated into the lacp object,
and that that would be more natural anyhow.

The old implementation of lacp_update_ifaces() called
ofproto_revalidate(), but the new equivalent lacp_update_attached()
function, understandably, does not.  Was the revalidation not really
needed?

In lacp_update_attached(), would you mind writing "lacp != NULL"
instead of "!!lacp"?  The !! notation bugs me, for no good reason.

In lacp_run(), would you mind changing the > to >= in the time_msec()
comparison?  Otherwise it seems plausible that poll_timer_wait_until()
would wake us up at exactly the expiration time and that we'd have to
go around the poll loop extra times (at 100% CPU) for up to a
millisecond before the expiration really takes effect.  Maybe that's
unlikely but...

Would you mind making send_pdu a parameter of lacp_run(), instead of a
member in struct lacp?  I see that lacp_run() is currently the only
place that the callback is called, but I am always nervous about
callbacks getting fired off in contexts where the caller does not
expect it.  Passing the callback only to the function that can
actually call it makes it really clear that that is the only context
that has to worry about the callback getting invoked.

In a lot of places in lacp.c, slave->tx is set to 0.  Does this value
represent "no tx expiration scheduled" or does it represent "immediate
tx expiration"?  If it represents the latter, then please use
LLONG_MIN instead of 0, because it makes it obvious that time_msec()
will always return a value at least that big.  (Before we switched to
the monotonic clock, it was also possible that the current time was
less than 0 if the system's time was set wrong, which is why I got in
the habit of using LLONG_MIN.  But that shouldn't happen anymore,
because POSIX says that the monotonic clock is always positive.)