As the result of an offline discussion, I'm planning not to commit this patch. It's solving an edge case problem that we don't really need to handle, and it's a bit messy.
Ethan On Wed, Mar 16, 2011 at 12:23 PM, Ben Pfaff <[email protected]> wrote: > On Tue, Mar 15, 2011 at 04:03:23PM -0700, Ethan Jackson wrote: >> When a slave may no longer participate in a LACP bond, it is polite >> to transmit a notification indicating so. This will allow the >> remote partner to adapt more quickly. The specification does not >> indicate what to do in this situation. This patch emulates the >> Linux bonding stack. > > Now that I look at the paths along which lacp_slave_unregister() is > called more carefully, I don't think that it is safe to assume that > slave_lookup() will always return nonnull here. I think that it > should just return immediately in that case. (That needs to be > applied to patch 7, I think.) > > The slave_lookup() at the end of lacp_slave_unregister() is now > redundant, I think. I think that the last line could just be > slave_destroy(slave); > > I don't think that it's safe to call the callback unconditionally > here. Just after the call to lacp_slave_unregister() in > iface_destroy() is an 'if' statement that checks whether netdev is > nonnull. But lacp_send_pdu_cb() does not check whether netdev is > nonnull. > > It might be better to just have lacp_slave_unregister() initialize a > pdu passed in by the caller, who can then do with it whatever it > wants. > > Thanks, > > Ben. > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
