[ovs-dev] [PATCH 07/10] lib/odp: Use masked set actions.

Ben Pfaff blp at nicira.com
Wed Apr 9 20:51:38 UTC 2014


On Wed, Apr 09, 2014 at 01:46:48PM -0700, Jarno Rajahalme wrote:
> > There is a doubled ';' in commit_set_ipv4_action():
> > +            key.ipv4_src = base->nw_src & wc->masks.nw_src;;
> > 
> > The beginning of commit_set_ipv4_action() first asserts:
> >        !((a ^ b) & ~c)
> > and then checks whether
> >        (a ^ b) & c
> > for several instances of 'a', 'b', and 'c'.  But if the assertions are
> > true, then I believe that the checks can be reduced to just
> >        a != b
> > because we already know at that the bits not in 'c' are the same.  (Am
> > I missing something?)
> > 
> 
> I had intended to remove the assertions. However, those have been
> valuable in catching cases where flow values have been modified
> without setting the mask bits. One way of keeping them around for
> testing purposes would be to piggyback on the FLOW_WC_SEQ, in effect
> enabling the asserts every time FLOW_WC_SEQ is bumped? other ideas?

I don't really object to the assertions, more to the masking in the
check given that we already know (either because of the assertion, or
because we assume that our code is correct) that the mask bits are the
same.  In other words, I'd just write a != b instead of (a ^ b) & c.
A lot of the functions already do a != b so I'm curious why
commit_set_ipv4_action() and maybe a few other cases are different.



More information about the dev mailing list