On Tue, Nov 08, 2011 at 03:57:29PM -0800, Justin Pettit wrote:
> This will be useful later when we add support for matching the ECN bits
> within the TOS field.
> 
> Signed-off-by: Justin Pettit <jpet...@nicira.com>

I think that these |=s can become =s:

>               if (offset) {
> -                     key->ip.tos_frag |= OVS_FRAG_TYPE_LATER;
> +                     key->ip.frag |= OVS_FRAG_TYPE_LATER;
>                       goto out;
>               }
>               if (nh->frag_off & htons(IP_MF) ||
>                        skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> -                     key->ip.tos_frag |= OVS_FRAG_TYPE_FIRST;
> +                     key->ip.frag |= OVS_FRAG_TYPE_FIRST;
>  
>               /* Transport layer. */
>               if (key->ip.proto == IPPROTO_TCP) {
> @@ -764,10 +762,10 @@ int flow_extract(struct sk_buff *skb, u16 in_port, 
> struct sw_flow_key *key,
>                       goto out;
>               }
>  
> -             if ((key->ip.tos_frag & OVS_FRAG_TYPE_MASK) == 
> OVS_FRAG_TYPE_LATER)
> +             if (key->ip.frag == OVS_FRAG_TYPE_LATER)
>                       goto out;
>               if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> -                     key->ip.tos_frag |= OVS_FRAG_TYPE_FIRST;
> +                     key->ip.frag |= OVS_FRAG_TYPE_FIRST;
>  
>               /* Transport layer. */
>               if (key->ip.proto == NEXTHDR_TCP) {

Did you mean to update the comment on 'frag' below?

>  struct sw_flow_key {
>       struct {
>               __be64  tun_id;         /* Encapsulating tunnel ID. */
> @@ -48,8 +44,8 @@ struct sw_flow_key {
>       } eth;
>       struct {
>               u8     proto;           /* IP protocol or lower 8 bits of ARP 
> opcode. */
> -             u8     tos_frag;        /* IP ToS DSCP in high 6 bits,
> -                                      * OVS_FRAG_TYPE_* in low 2 bits. */
> +             u8     tos;         /* IP ToS DSCP in high 6 bits. */
> +             u8     frag;        /* OVS_FRAG_TYPE_* in low 2 bits. */
>       } ip;
>       union {
>               struct {

In struct, could we put the 'frag' byte after arp_tha so that arp_sha
and arp_tha don't start on odd addresses:

> @@ -70,21 +70,23 @@ struct flow {
>      uint8_t dl_src[6];          /* Ethernet source address. */
>      uint8_t dl_dst[6];          /* Ethernet destination address. */
>      uint8_t nw_proto;           /* IP protocol or low 8 bits of ARP opcode. 
> */
> -    uint8_t tos_frag;           /* IP ToS in top bits, FLOW_FRAG_* in low. */
> +    uint8_t tos;                /* IP ToS. */
> +    uint8_t frag;               /* FLOW_FRAG_*. */
>      uint8_t arp_sha[6];         /* ARP/ND source hardware address. */
>      uint8_t arp_tha[6];         /* ARP/ND target hardware address. */
> +    uint8_t reserved[7];        /* Reserved for 64-bit packing. */
>  };

In nx-match.c, I think that it might be a little better to keep the
nxm_put_tos_frag() function together just to reduce code duplication.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to