> 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 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
