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

[Ethan Jackson]

> 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.

This is actually fairly difficult to get rid of.  In order to remove
it we would need to move the lacp_configure() function call into
port_reconfigure() so that we don't need store the lacp_active
configuration parameter until port_update_lacp().   Unfortunately, we
can't really move lacp_configure into port_reconfigure because it
requires port->bridge->ea which is not initialized yet.  This is
certainly not an insurmountable problem.  However lacp will shortly be
moving out of the bridge and the lacp_active parameter will go with.
I'm inclined to leave it until then.

> 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?

It wasn't really needed.  Flows need to be revalidated when a slave's
enabled status changes.  Lacp attached status may cause a given
slave's enabled status to change in which case it's flows will be
revalidated.  But there doesn't exist a case where the attached status
changes, the enabled status doesn't change, and flows need to be
revalidated.  This is because attached status really has nothing to do
with flows directly.

> 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.

This is a good idea.  Cleaner.

An incremental is on it's way.
Ethan