[ovs-dev] [nxm 41/42] Add support for matching Ethernet multicast frames.

[Justin Pettit]

On Oct 28, 2010, at 10:28 AM, Ben Pfaff wrote:

> @@ -291,6 +291,9 @@ flow_from_match(const struct ofp_match *match, int flow_format,
>             flow->tun_id = htonl(ntohll(cookie) >> 32);
>         }
>     }
> +    if (wildcards & OFPFW_DL_DST) {
> +        wildcards |= FWW_ETH_MCAST;
> +    }

It may be nice to point out that the OpenFlow 1.0 ofp_match structure doesn't have a concept of masking Ethernet destination addresses, but our internal way of supporting it requires us to patch these places up.

> +/* Set to 1 if bit 0 (the multicast bit) of the flow's dl_dst is wildcarded.
> + *
> + * (We reinterpret OFPFW_DL_DST as excluding bit 0.) */

I know it's probably obvious, but I think it would be clearer to indicate that (FWW_ETH_MCAST | OFPFW_DL_DST) needs to be set to wildcard all destination MAC addresses.

> +#define FWW_ETH_MCAST (1u << 30)

I feel like we're on our way to a stack/hash collision.  :-)

> +static void
> +nxm_put_eth_dst(struct ofpbuf *b,
> +                uint32_t wc, const uint8_t value[ETH_ADDR_LEN])
> +{
> +    switch (wc & (OFPFW_DL_DST | FWW_ETH_MCAST)) {
> ...
> +    case 0:
> +        nxm_put_header(b, NXM_OF_ETH_DST);
> +        ofpbuf_put(b, value, ETH_ADDR_LEN);
> +        break;

The function nxm_put_eth() does what's in that case already.  After this, the only caller of that function is for setting the source Ethernet address.  It would be nice to either use that function here or retire it, since there's not much there.

> --- a/tests/ovs-ofctl.at
> +++ b/tests/ovs-ofctl.at
> @@ -36,6 +36,10 @@ NXM_OF_IN_PORT(fffe)
> 
> # eth dst
> NXM_OF_ETH_DST(0002e30f80a4)
> +NXM_OF_ETH_DST_W(010000000000/010000000000)
> +NXM_OF_ETH_DST_W(000000000000/010000000000)
> +NXM_OF_ETH_DST_W(0002e30f80a4/ffffffffffff)
> +NXM_OF_ETH_DST_W(0002e30f80a4/feffffffffff)
> 
> # eth src
> NXM_OF_ETH_SRC(020898456ddb)
> @@ -137,6 +141,10 @@ NXM_OF_IN_PORT(fffe)
> 
> # eth dst
> NXM_OF_ETH_DST(0002e30f80a4)
> +NXM_OF_ETH_DST_W(010000000000/010000000000)
> +NXM_OF_ETH_DST_W(000000000000/010000000000)
> +NXM_OF_ETH_DST(0002e30f80a4)
> +NXM_OF_ETH_DST_W(0002e30f80a4/feffffffffff)
> 
> # eth src
> NXM_OF_ETH_SRC(020898456ddb)

Do you think it's worth having some tests that check that masked values get properly set to zero in the resulting output?  It seems like it would be good to enforce some of the invariants defined in flow.h.

--Justin