[ovs-dev] [generic tci mask 4/8] flow: Move functions for dealing with wildcard bit counts to ofp-util.

[Ben Pfaff]

On Fri, Nov 19, 2010 at 02:55:00PM -0800, Justin Pettit wrote:
> On Nov 10, 2010, at 3:37 PM, Ben Pfaff wrote:
> 
> > +/* Given the wildcard bit count in bits 0...5 (inclusive) of 'wildcards',
> > + * returns an IP netmask with a 1 in each bit that must match and a 0 in each
> > + * bit that is wildcarded.
> > + *
> > + * The bits in 'wildcards' are in the format used in enum ofp_flow_wildcards: 0
> > + * is exact match, 1 ignores the LSB, 2 ignores the 2 least-significant bits,
> > + * ..., 32 and higher wildcard the entire field.  This is the *opposite* of the
> > + * usual convention where e.g. /24 indicates that 8 bits (not 24 bits) are
> > + * wildcarded. */
> 
> The comment references "wildcards", but the argument was changed to "wcbits".

Oops.  I changed the parameter name here about a dozen times.  In the
end I found 'wcbits' least confusing.

Fixed.

> > +ovs_be32
> > +ofputil_wcbits_to_netmask(int wcbits)
> > +{
> > +    wcbits &= wcbits;
> 
> I think you can safely remove this line.  :-)

Thanks for pointing that out, it was supposed to be wcbits &= 0x3f.

> > +/* Given the IP netmask 'netmask', returns the number of bits of the IP address
> > + * that it wildcards.  The return value is only accurate if 'netmask' is a CIDR
> > + * netmask (see ip_is_cidr()). */
> > +int
> > +ofputil_netmask_to_wcbits(ovs_be32 netmask)
> > +{
> 
> Do you think it's worth making an assertion that ip_is_cidr() is true?

I guess it can't hurt.  Added.

> > +/* Returns true if 'netmask' is a CIDR netmask, that is, if it consists of N
> > + * high-order 1-bits and 32-N low-order 0-bits. */
> > +static inline bool
> > +ip_is_cidr(ovs_be32 netmask)
> > +{
> > +    uint32_t x = ~ntohl(netmask);
> > +    return !(x & (x + 1));
> > +}
> 
> Clever.  :-)

The older I get, the less clever my code gets, but sometimes I still
can't resist.

> > @@ -200,9 +201,12 @@ match(const struct cls_rule *wild, const struct flow *fixed)
> >         if (wild->wc.wildcards & f->wildcards) {
> >             uint32_t test = get_unaligned_u32(wild_field);
> >             uint32_t ip = get_unaligned_u32(fixed_field);
> > -            int shift = (f_idx == CLS_F_IDX_NW_SRC
> > +            uint32_t mask;
> > +            int shift;
> > +
> > +            shift = (f_idx == CLS_F_IDX_NW_SRC
> >                          ? OFPFW_NW_SRC_SHIFT : OFPFW_NW_DST_SHIFT);
> > -            uint32_t mask = flow_nw_bits_to_mask(wild->wc.wildcards, shift);
> > +            mask = ofputil_wcbits_to_netmask(wild->wc.wildcards) >> shift;
> 
> Should that shift be occurring within the parentheses?

Yes, thanks.  Oops!