[ovs-dev] [warnings 4/8] ofp-util: Simplify OpenFlow 1.0 ofp_match normalization.

[Ethan Jackson]

Looks Good.

Ethan

On Mon, May 2, 2011 at 12:58, Ben Pfaff <blp at nicira.com> wrote:
> The normalize_match() function does more work than really needed.  It goes
> to some trouble to zero out fields that are wildcarded.  This is not
> necessary, because cls_rule_from_match() will take care of it later.
>
> Also make normalize_match() private to ofp-util.c, since it has no other
> users now and I don't expect more later.
> ---
>  lib/ofp-util.c |   58 +++++++------------------------------------------------
>  lib/ofp-util.h |    1 -
>  2 files changed, 8 insertions(+), 51 deletions(-)
>
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index ade0756..7772c47 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -36,6 +36,8 @@
>
>  VLOG_DEFINE_THIS_MODULE(ofp_util);
>
> +static void normalize_match(struct ofp_match *);
> +
>  /* Rate limit for OpenFlow message parse errors.  These always indicate a bug
>  * in the peer and so there's not much point in showing a lot of them. */
>  static struct vlog_rate_limit bad_ofmsg_rl = VLOG_RATE_LIMIT_INIT(1, 5);
> @@ -2028,7 +2030,7 @@ actions_next(struct actions_iterator *iter)
>     }
>  }
>
> -void
> +static void
>  normalize_match(struct ofp_match *m)
>  {
>     enum { OFPFW_NW = (OFPFW_NW_SRC_ALL | OFPFW_NW_DST_ALL | OFPFW_NW_PROTO
> @@ -2038,74 +2040,30 @@ normalize_match(struct ofp_match *m)
>
>     wc = ntohl(m->wildcards) & OFPFW_ALL;
>     if (wc & OFPFW_DL_TYPE) {
> -        m->dl_type = 0;
> -
>         /* Can't sensibly match on network or transport headers if the
>          * data link type is unknown. */
>         wc |= OFPFW_NW | OFPFW_TP;
> -        m->nw_src = m->nw_dst = m->nw_proto = m->nw_tos = 0;
> -        m->tp_src = m->tp_dst = 0;
>     } else if (m->dl_type == htons(ETH_TYPE_IP)) {
>         if (wc & OFPFW_NW_PROTO) {
> -            m->nw_proto = 0;
> -
>             /* Can't sensibly match on transport headers if the network
>              * protocol is unknown. */
>             wc |= OFPFW_TP;
> -            m->tp_src = m->tp_dst = 0;
> -        } else if (m->nw_proto == IPPROTO_TCP ||
> -                   m->nw_proto == IPPROTO_UDP ||
> -                   m->nw_proto == IPPROTO_ICMP) {
> -            if (wc & OFPFW_TP_SRC) {
> -                m->tp_src = 0;
> -            }
> -            if (wc & OFPFW_TP_DST) {
> -                m->tp_dst = 0;
> -            }
> -        } else {
> +        } else if (m->nw_proto != IPPROTO_TCP &&
> +                   m->nw_proto != IPPROTO_UDP &&
> +                   m->nw_proto != IPPROTO_ICMP) {
>             /* Transport layer fields will always be extracted as zeros, so we
>              * can do an exact-match on those values.  */
>             wc &= ~OFPFW_TP;
>             m->tp_src = m->tp_dst = 0;
>         }
> -        if (wc & OFPFW_NW_SRC_MASK) {
> -            m->nw_src &= ofputil_wcbits_to_netmask(wc >> OFPFW_NW_SRC_SHIFT);
> -        }
> -        if (wc & OFPFW_NW_DST_MASK) {
> -            m->nw_dst &= ofputil_wcbits_to_netmask(wc >> OFPFW_NW_DST_SHIFT);
> -        }
> -        if (wc & OFPFW_NW_TOS) {
> -            m->nw_tos = 0;
> -        } else {
> -            m->nw_tos &= IP_DSCP_MASK;
> -        }
> -    } else if (m->dl_type == htons(ETH_TYPE_ARP)) {
> -        if (wc & OFPFW_NW_PROTO) {
> -            m->nw_proto = 0;
> -        }
> -        if (wc & OFPFW_NW_SRC_MASK) {
> -            m->nw_src &= ofputil_wcbits_to_netmask(wc >> OFPFW_NW_SRC_SHIFT);
> -        }
> -        if (wc & OFPFW_NW_DST_MASK) {
> -            m->nw_dst &= ofputil_wcbits_to_netmask(wc >> OFPFW_NW_DST_SHIFT);
> -        }
> -        m->tp_src = m->tp_dst = m->nw_tos = 0;
> -    } else if (m->dl_type == htons(ETH_TYPE_IPV6)) {
> -        /* Don't normalize IPv6 traffic, since OpenFlow doesn't have a
> -         * way to express it. */
> -    } else {
> +    } else if (m->dl_type != htons(ETH_TYPE_ARP) &&
> +               m->dl_type != htons(ETH_TYPE_IPV6)) {
>         /* Network and transport layer fields will always be extracted as
>          * zeros, so we can do an exact-match on those values. */
>         wc &= ~(OFPFW_NW | OFPFW_TP);
>         m->nw_proto = m->nw_src = m->nw_dst = m->nw_tos = 0;
>         m->tp_src = m->tp_dst = 0;
>     }
> -    if (wc & OFPFW_DL_SRC) {
> -        memset(m->dl_src, 0, sizeof m->dl_src);
> -    }
> -    if (wc & OFPFW_DL_DST) {
> -        memset(m->dl_dst, 0, sizeof m->dl_dst);
> -    }
>     m->wildcards = htonl(wc);
>  }
>
> diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> index 6e69bae..8904404 100644
> --- a/lib/ofp-util.h
> +++ b/lib/ofp-util.h
> @@ -101,7 +101,6 @@ int ofputil_netmask_to_wcbits(ovs_be32 netmask);
>  void ofputil_cls_rule_from_match(const struct ofp_match *,
>                                  unsigned int priority, struct cls_rule *);
>  void ofputil_cls_rule_to_match(const struct cls_rule *, struct ofp_match *);
> -void normalize_match(struct ofp_match *);
>  char *ofp_match_to_literal_string(const struct ofp_match *match);
>
>  /* dl_type translation between OpenFlow and 'struct flow' format. */
> --
> 1.7.4.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>