[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openvswitch-discuss
Subject:    [ovs-discuss] [ACLv2 12/19] flow: Allow matching on additional ARP payload fields.
From:       blp () nicira ! com (Ben Pfaff)
Date:       2009-08-25 20:23:32
Message-ID: 873a7fzlq3.fsf () blp ! benpfaff ! org
[Download RAW message or body]

Jesse Gross <jesse at nicira.com> writes:

> @@ -53,7 +53,10 @@
>      CLS_FIELD(OFPFW_NW_DST_MASK, nw_dst,      NW_DST)       \
>      CLS_FIELD(OFPFW_NW_PROTO,    nw_proto,    NW_PROTO)     \
>      CLS_FIELD(OFPFW_TP_SRC,      tp_src,      TP_SRC)       \
> -    CLS_FIELD(OFPFW_TP_DST,      tp_dst,      TP_DST)
> +    CLS_FIELD(OFPFW_TP_DST,      tp_dst,      TP_DST)       \
> +    CLS_FIELD(NICFW_AR_SHA,      ar_sha,      AR_SHA)       \
> +    CLS_FIELD(NICFW_AR_THA,      ar_tha,      AR_THA)
> +

I'm not a huge fan of adding two more fields to the classifier
table.  I think that tp_src and tp_dst are mutually exclusive
with ar_sha and ar_tha.  Is that correct?  If so, is there any
way that we can just extend tp_src and tp_dst to 6 bytes each and
keep the current number of classifier fields?

I don't see any updates to tests/test-classifier.c, but I think
that some would be required, perhaps just to get it to compile
and otherwise to make sure that the new fields are tested.  (Does
this pass "make check"?  How does the code coverage for
classifier.c look if you build with --enable-coverage?  We get
82% coverage at the moment and I'd like to see that go up, not
down.)

> @@ -238,13 +240,13 @@ flow_extract_stats(const flow_t *flow, const struct ofpbuf *packet,
>  void
>  flow_to_match(const flow_t *flow, uint32_t wildcards, struct ofp_match *match)
>  {
> -    /* The datapath supports matching on an ARP's opcode and IP addresses, 
> -     * but OpenFlow does not.  We need to clear the appropriate wildcard 
> -     * bits so that OpenFlow is unaware of our trickery. */
> +    /* We support matching on an ARP's payload, but OpenFlow does not.
> +     * We need to clear the appropriate wildcard bits so that OpenFlow
> +     * is unaware of our trickery. */
>      if (flow->dl_type == htons(ETH_TYPE_ARP)) {
> -        wildcards = ~(OFPFW_NW_PROTO | OFPFW_NW_SRC_ALL | OFPFW_NW_DST_ALL) 
> -                    & wildcards;
> +        wildcards &= ~(OFPFW_NW_PROTO | OFPFW_NW_SRC_ALL | OFPFW_NW_DST_ALL);
Heh, there's the &= that I suggested for an earlier commit :-)

>      }
> +    wildcards &= ~(NICFW_AR_SHA | NICFW_AR_THA);
>      match->wildcards = htonl(wildcards);
>  
>      match->in_port = htons(flow->in_port == ODPP_LOCAL ? OFPP_LOCAL

> diff --git a/lib/flow.h b/lib/flow.h
> index 0932746..d110213 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -30,6 +30,13 @@ struct ds;
>  struct ofp_match;
>  struct ofpbuf;
>  
> +enum nicira_flow_wildcards {
> +    NICFW_AR_SHA  = 1 << 25,  /* ARP sender hardware address. */
> +    NICFW_AR_THA  = 1 << 26   /* ARP target hardware address. */
> +};

Can we use ovs_flow_wildcards and OVSFW_*?  After all this is
Open vSwitch code, not anything really Nicira-specific.

> +#define OFPFW_ALL (OFPFW_ALL | NICFW_AR_SHA | NICFW_AR_THA)

I don't think that we should redefine OFPFW_ALL, especially not
in terms of itself.  That's sure to confuse people and cause
bizarre problems for anything that includes openflow.h but not
flow.h.  How about OVSFW_ALL?

>  struct userspace_flow {

Now I'm off to see where struct userspace_flow came from.  I
think I missed a commit somewhere.


[prev in list] [next in list] [prev in thread] [next in thread]