[ovs-dev] [lacp 10/10] lacp: Transmit upon slave unregister.

[Ethan Jackson]

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 <blp at nicira.com> 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.
>