[ovs-dev] [PATCH v2 10/12] nx-match: Add support for experimenter OXM.

[Ben Pfaff]

On Tue, Oct 07, 2014 at 10:58:55AM +0900, YAMAMOTO Takashi wrote:
> > -#define NXM_HEADER(CLASS, FIELD, HASMASK, LENGTH)                       \
> > -    (((CLASS) << 16) | ((FIELD) << 9) | ((HASMASK) << 8) | (LENGTH))
> > +#define NXM_HEADER(VENDOR, CLASS, HASMASK, FIELD, LENGTH)       \
> 
> why did you swap FIELD and HASMASK?
> it's easier for me to remember if it's in the on-wire order.
> ie. CLASS, FIELD, HASMASK, LENGTH, VENDOR.

I don't know why this happened.  I agree that the on-wire order is
better.  I made that change.

> >  static void
> > -nx_put_header__(struct ofpbuf *b, uint32_t header, bool masked)
> > +nx_put_header__(struct ofpbuf *b, uint64_t header, bool masked)
> >  {
> > -    uint32_t masked_header = masked ? nxm_make_wild_header(header) : header;
> > -    ovs_be32 network_header = htonl(masked_header);
> > +    uint64_t masked_header = masked ? nxm_make_wild_header(header) : header;
> > +    ovs_be64 network_header = htonll(masked_header);
> >  
> > -    ofpbuf_put(b, &network_header, sizeof network_header);
> > +    ofpbuf_put(b, &network_header, 4 + nxm_experimenter_len(header));
> 
> better to use nxm_header_len?

I agree.  I made that change too.  Here is the incremental that I
folded in.  Because Jarno acked this, and because you did not see any
serious problems, I applied this to master.

Thanks,

ben.

diff --git a/build-aux/extract-ofp-fields b/build-aux/extract-ofp-fields
index 88eb6d2..95714ee 100755
--- a/build-aux/extract-ofp-fields
+++ b/build-aux/extract-ofp-fields
@@ -143,7 +143,7 @@ def parse_oxm(s, prefix, n_bytes):
     if oxm_class == 0xffff:
         oxm_length += 4
 
-    header = ("NXM_HEADER(0x%x,0x%x,0,%s,%d)"
+    header = ("NXM_HEADER(0x%x,0x%x,%s,0,%d)"
               % (oxm_vendor, oxm_class, oxm_type, oxm_length))
 
     if of_version:
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 13e758b..82b472c 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -148,7 +148,7 @@ is_nxm_header(uint64_t header)
     return nxm_class(header) <= 1;
 }
 
-#define NXM_HEADER(VENDOR, CLASS, HASMASK, FIELD, LENGTH)       \
+#define NXM_HEADER(VENDOR, CLASS, FIELD, HASMASK, LENGTH)       \
     (((uint64_t) (CLASS) << 48) |                               \
      ((uint64_t) (FIELD) << 41) |                               \
      ((uint64_t) (HASMASK) << 40) |                             \
@@ -166,15 +166,15 @@ static uint64_t
 nxm_make_exact_header(uint64_t header)
 {
     int new_len = nxm_payload_len(header) / 2 + nxm_experimenter_len(header);
-    return NXM_HEADER(nxm_vendor(header), nxm_class(header), 0,
-                      nxm_field(header), new_len);
+    return NXM_HEADER(nxm_vendor(header), nxm_class(header),
+                      nxm_field(header), 0, new_len);
 }
 static uint64_t
 nxm_make_wild_header(uint64_t header)
 {
     int new_len = nxm_payload_len(header) * 2 + nxm_experimenter_len(header);
-    return NXM_HEADER(nxm_vendor(header), nxm_class(header), 1,
-                      nxm_field(header), new_len);
+    return NXM_HEADER(nxm_vendor(header), nxm_class(header),
+                      nxm_field(header), 1, new_len);
 }
 
 /* Flow cookie.
@@ -184,7 +184,7 @@ nxm_make_wild_header(uint64_t header)
  * with specific cookies.  See the "nx_flow_mod" and "nx_flow_stats_request"
  * structure definitions for more details.  This match is otherwise not
  * allowed. */
-#define NXM_NX_COOKIE     NXM_HEADER  (0, 0x0001, 0, 30, 8)
+#define NXM_NX_COOKIE     NXM_HEADER  (0, 0x0001, 30, 0, 8)
 #define NXM_NX_COOKIE_W   nxm_make_wild_header(NXM_NX_COOKIE)
 
 struct nxm_field {
@@ -1040,7 +1040,7 @@ nx_put_header__(struct ofpbuf *b, uint64_t header, bool masked)
     uint64_t masked_header = masked ? nxm_make_wild_header(header) : header;
     ovs_be64 network_header = htonll(masked_header);
 
-    ofpbuf_put(b, &network_header, 4 + nxm_experimenter_len(header));
+    ofpbuf_put(b, &network_header, nxm_header_len(header));
 }
 
 void