[ovs-dev] [PATCH 5/5] vswitchd: LACP switch lacp_status to bit mask.

Ben Pfaff blp at nicira.com
Mon Feb 7 19:30:21 UTC 2011


On Fri, Feb 04, 2011 at 06:35:50PM -0800, Ethan Jackson wrote:
> Much of the LACP status information attached to an interface is
> moved from an enum to a bit mask in this commit.  The main reason
> to do this is to allow a link to be concurrently expired and
> defaulted.  With this commit, if a link enters an expired state,
> but has never had its partner information update, it will properly
> set the defaulted flag in its LACP messages.

Looks good, but I have a few nitpicks.

I liked the longer names.  They were easier to understand at a glance.
Would you mind spelling them out again, e.g. maybe LACP_CURRENT,
LCAP_EXPIRED, LACP_DEFAULTED, LACP_ATTACHED?

Usually we've been using an enum for this kind of thing, instead of
macros.  Occasionally this is convenient in GDB, for example.  

I see a few instances of 'x & a || x & b'.  Would you mind writing these
as 'x & (a | b)'?  I guess that GCC probably outputs exactly the same
code in each case but the latter seems more canonical to me.

Thanks,

Ben.




More information about the dev mailing list