diff mbox

[ovs-dev] flow: pass last field to miniflow_pad_to_64

Message ID 1448949796-16119-1-git-send-email-simon.horman@netronome.com
State Accepted
Headers show

Commit Message

Simon Horman Dec. 1, 2015, 6:03 a.m. UTC 
Make miniflow_pad_to_64() a little more robust with regards to updates to
struct flow by passing the last field, whose end should be considered for
padding, rather than the next field, whose start should be considered.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 lib/flow.c | 18 +++++++++---------
 lib/util.h |  4 ++++
 2 files changed, 13 insertions(+), 9 deletions(-)

Comments

Ben Pfaff Dec. 1, 2015, 6:29 p.m. UTC | #1 
On Tue, Dec 01, 2015 at 03:03:16PM +0900, Simon Horman wrote:
> Make miniflow_pad_to_64() a little more robust with regards to updates to
> struct flow by passing the last field, whose end should be considered for
> padding, rather than the next field, whose start should be considered.
> 
> Signed-off-by: Simon Horman <simon.horman@netronome.com>

Jarno, will you review this?

Thanks,

Ben.
Jarno Rajahalme Dec. 17, 2015, 12:55 a.m. UTC | #2 
Sorry Ben,

I forgot about this and now I ran out of time.

  Jarno

> On Dec 1, 2015, at 10:29 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Tue, Dec 01, 2015 at 03:03:16PM +0900, Simon Horman wrote:
>> Make miniflow_pad_to_64() a little more robust with regards to updates to
>> struct flow by passing the last field, whose end should be considered for
>> padding, rather than the next field, whose start should be considered.
>> 
>> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> 
> Jarno, will you review this?
> 
> Thanks,
> 
> Ben.
Ben Pfaff Dec. 23, 2015, 6:39 a.m. UTC | #3 
On Tue, Dec 01, 2015 at 03:03:16PM +0900, Simon Horman wrote:
> Make miniflow_pad_to_64() a little more robust with regards to updates to
> struct flow by passing the last field, whose end should be considered for
> padding, rather than the next field, whose start should be considered.
> 
> Signed-off-by: Simon Horman <simon.horman@netronome.com>

I asked Jarno to review this, but then he asked me right back.

I've now read it and I like it very much.  It makes it harder to write
the wrong code, and I'm all in favor of that.

I applied this to master.  Thank you!
diff mbox

Patch

diff --git a/lib/flow.c b/lib/flow.c
index 2b0c6d2e3179..f22c0aa98502 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -263,7 +263,7 @@  BUILD_MESSAGE("FLOW_WC_SEQ changed: miniflow_extract() will have runtime "
     miniflow_push_be16_(MF, offsetof(struct flow, FIELD), VALUE)
 
 #define miniflow_pad_to_64(MF, FIELD)                       \
-    miniflow_pad_to_64_(MF, offsetof(struct flow, FIELD))
+    miniflow_pad_to_64_(MF, OFFSETOFEND(struct flow, FIELD))
 
 #define miniflow_push_words(MF, FIELD, VALUEP, N_WORDS)                 \
     miniflow_push_words_(MF, offsetof(struct flow, FIELD), VALUEP, N_WORDS)
@@ -485,7 +485,7 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
 
     if (md->ct_state) {
         miniflow_push_uint32(mf, ct_mark, md->ct_mark);
-        miniflow_pad_to_64(mf, pad1);
+        miniflow_pad_to_64(mf, ct_mark);
 
         if (!ovs_u128_is_zero(&md->ct_label)) {
             miniflow_push_words(mf, ct_label, &md->ct_label,
@@ -695,7 +695,7 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
                 arp_buf[0] = arp->ar_sha;
                 arp_buf[1] = arp->ar_tha;
                 miniflow_push_macs(mf, arp_sha, arp_buf);
-                miniflow_pad_to_64(mf, tcp_flags);
+                miniflow_pad_to_64(mf, arp_tha);
             }
         }
         goto out;
@@ -715,7 +715,7 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
                                    TCP_FLAGS_BE32(tcp->tcp_ctl));
                 miniflow_push_be16(mf, tp_src, tcp->tcp_src);
                 miniflow_push_be16(mf, tp_dst, tcp->tcp_dst);
-                miniflow_pad_to_64(mf, igmp_group_ip4);
+                miniflow_pad_to_64(mf, tp_dst);
             }
         } else if (OVS_LIKELY(nw_proto == IPPROTO_UDP)) {
             if (OVS_LIKELY(size >= UDP_HEADER_LEN)) {
@@ -723,7 +723,7 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
 
                 miniflow_push_be16(mf, tp_src, udp->udp_src);
                 miniflow_push_be16(mf, tp_dst, udp->udp_dst);
-                miniflow_pad_to_64(mf, igmp_group_ip4);
+                miniflow_pad_to_64(mf, tp_dst);
             }
         } else if (OVS_LIKELY(nw_proto == IPPROTO_SCTP)) {
             if (OVS_LIKELY(size >= SCTP_HEADER_LEN)) {
@@ -731,7 +731,7 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
 
                 miniflow_push_be16(mf, tp_src, sctp->sctp_src);
                 miniflow_push_be16(mf, tp_dst, sctp->sctp_dst);
-                miniflow_pad_to_64(mf, igmp_group_ip4);
+                miniflow_pad_to_64(mf, tp_dst);
             }
         } else if (OVS_LIKELY(nw_proto == IPPROTO_ICMP)) {
             if (OVS_LIKELY(size >= ICMP_HEADER_LEN)) {
@@ -739,7 +739,7 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
 
                 miniflow_push_be16(mf, tp_src, htons(icmp->icmp_type));
                 miniflow_push_be16(mf, tp_dst, htons(icmp->icmp_code));
-                miniflow_pad_to_64(mf, igmp_group_ip4);
+                miniflow_pad_to_64(mf, tp_dst);
             }
         } else if (OVS_LIKELY(nw_proto == IPPROTO_IGMP)) {
             if (OVS_LIKELY(size >= IGMP_HEADER_LEN)) {
@@ -762,10 +762,10 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
                                         sizeof *nd_target / sizeof(uint64_t));
                 }
                 miniflow_push_macs(mf, arp_sha, arp_buf);
-                miniflow_pad_to_64(mf, tcp_flags);
+                miniflow_pad_to_64(mf, arp_tha);
                 miniflow_push_be16(mf, tp_src, htons(icmp->icmp6_type));
                 miniflow_push_be16(mf, tp_dst, htons(icmp->icmp6_code));
-                miniflow_pad_to_64(mf, igmp_group_ip4);
+                miniflow_pad_to_64(mf, tp_dst);
             }
         }
     }
diff --git a/lib/util.h b/lib/util.h
index 340ef65f07ab..6345247d020c 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -202,6 +202,10 @@  ovs_prefetch_range(const void *start, size_t size)
 /* Yields the size of MEMBER within STRUCT. */
 #define MEMBER_SIZEOF(STRUCT, MEMBER) (sizeof(((STRUCT *) NULL)->MEMBER))
 
+/* Yields the offset of the end of MEMBER within STRUCT. */
+#define OFFSETOFEND(STRUCT, MEMBER) \
+        (offsetof(STRUCT, MEMBER) + MEMBER_SIZEOF(STRUCT, MEMBER))
+
 /* Given POINTER, the address of the given MEMBER in a STRUCT object, returns
    the STRUCT object. */
 #define CONTAINER_OF(POINTER, STRUCT, MEMBER)                           \