ofp-util: Consistently treat OpenFlow xids as network byte order.

The 'xid' in an ofp_header is not interpreted by the receiver but only by
the sender, so it need not be in any particular byte order.  OVS used to
try to take advantage of this to avoid host/network byte order conversions
for this field.  Older code in OVS, therefore, treats xid as being in host
byte order.  However, as time went on, I forgot that I had introduced this
trick, and so newer code treats xid as being in network byte order.

This commit fixes up the situation by consistently treating xid as being
in network byte order.  I think that this will be less surprising and
easier to remember in the future.

This doesn't fix any actual bugs except that some log messages would have
printed xids in the wrong byte order.

diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 4572db4..507ed7d 100644 (file)
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -1596,7 +1596,7 @@ ofp_to_string(const void *oh_, size_t len, int verbosity)
         }
     }
 
-    ds_put_format(&string, "%s (xid=0x%"PRIx32"):", pkt->name, oh->xid);
+    ds_put_format(&string, "%s (xid=0x%"PRIx32"):", pkt->name, ntohl(oh->xid));
 
     if (ntohs(oh->length) > len)
         ds_put_format(&string, " (***truncated to %zu bytes from %"PRIu16"***)",

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 8a9a8ec..5c4336a 100644 (file)
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -33,11 +33,11 @@ VLOG_DEFINE_THIS_MODULE(ofp_util);
 static struct vlog_rate_limit bad_ofmsg_rl = VLOG_RATE_LIMIT_INIT(1, 5);
 
 /* Returns a transaction ID to use for an outgoing OpenFlow message. */
-static uint32_t
+static ovs_be32
 alloc_xid(void)
 {
     static uint32_t next_xid = 1;
-    return next_xid++;
+    return htonl(next_xid++);
 }
 
 /* Allocates and stores in '*bufferp' a new ofpbuf with a size of
@@ -82,7 +82,7 @@ make_nxmsg(size_t openflow_len, uint32_t subtype, struct ofpbuf **bufferp)
  *
  * Returns the header. */
 void *
-make_openflow_xid(size_t openflow_len, uint8_t type, uint32_t xid,
+make_openflow_xid(size_t openflow_len, uint8_t type, ovs_be32 xid,
                   struct ofpbuf **bufferp)
 {
     *bufferp = ofpbuf_new(openflow_len);
@@ -92,7 +92,7 @@ make_openflow_xid(size_t openflow_len, uint8_t type, uint32_t xid,
 /* Similar to make_openflow_xid() but creates a Nicira vendor extension message
  * with the specific 'subtype'.  'subtype' should be in host byte order. */
 void *
-make_nxmsg_xid(size_t openflow_len, uint32_t subtype, uint32_t xid,
+make_nxmsg_xid(size_t openflow_len, uint32_t subtype, ovs_be32 xid,
                struct ofpbuf **bufferp)
 {
     struct nicira_header *nxh = make_openflow_xid(openflow_len, OFPT_VENDOR,
@@ -127,7 +127,7 @@ put_openflow(size_t openflow_len, uint8_t type, struct ofpbuf *buffer)
  *
  * Returns the header. */
 void *
-put_openflow_xid(size_t openflow_len, uint8_t type, uint32_t xid,
+put_openflow_xid(size_t openflow_len, uint8_t type, ovs_be32 xid,
                  struct ofpbuf *buffer)
 {
     struct ofp_header *oh;
@@ -308,7 +308,7 @@ make_echo_request(void)
     rq->version = OFP_VERSION;
     rq->type = OFPT_ECHO_REQUEST;
     rq->length = htons(sizeof *rq);
-    rq->xid = 0;
+    rq->xid = htonl(0);
     return out;
 }
 
@@ -852,7 +852,7 @@ make_ofp_error_msg(int error, const struct ofp_header *oh)
     uint8_t vendor;
     uint16_t type;
     uint16_t code;
-    uint32_t xid;
+    ovs_be32 xid;
 
     if (!is_ofp_error(error)) {
         /* We format 'error' with strerror() here since it seems likely to be

diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 3467366..05d6ace 100644 (file)
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -22,6 +22,7 @@
 #include <stddef.h>
 #include <stdint.h>
 #include "flow.h"
+#include "openvswitch/types.h"
 
 struct ofpbuf;
 struct ofp_action_header;
@@ -33,11 +34,11 @@ struct ofp_action_header;
 void *make_openflow(size_t openflow_len, uint8_t type, struct ofpbuf **);
 void *make_nxmsg(size_t openflow_len, uint32_t subtype, struct ofpbuf **);
 void *make_openflow_xid(size_t openflow_len, uint8_t type,
-                        uint32_t xid, struct ofpbuf **);
-void *make_nxmsg_xid(size_t openflow_len, uint32_t subtype, uint32_t xid,
+                        ovs_be32 xid, struct ofpbuf **);
+void *make_nxmsg_xid(size_t openflow_len, uint32_t subtype, ovs_be32 xid,
                      struct ofpbuf **);
 void *put_openflow(size_t openflow_len, uint8_t type, struct ofpbuf *);
-void *put_openflow_xid(size_t openflow_len, uint8_t type, uint32_t xid,
+void *put_openflow_xid(size_t openflow_len, uint8_t type, ovs_be32 xid,
                        struct ofpbuf *);
 void update_openflow_length(struct ofpbuf *);
 struct ofpbuf *make_flow_mod(uint16_t command, const struct flow *,

diff --git a/lib/vconn.c b/lib/vconn.c
index d2a3829..380e374 100644 (file)
--- a/lib/vconn.c
+++ b/lib/vconn.c
@@ -667,7 +667,8 @@ vconn_recv_xid(struct vconn *vconn, uint32_t xid, struct ofpbuf **replyp)
         }
 
         VLOG_DBG_RL(&bad_ofmsg_rl, "%s: received reply with xid %08"PRIx32
-                    " != expected %08"PRIx32, vconn->name, recv_xid, xid);
+                    " != expected %08"PRIx32,
+                    vconn->name, ntohl(recv_xid), ntohl(xid));
         ofpbuf_delete(reply);
     }
 }

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index e46e1a2..be68bdc 100644 (file)
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -307,7 +307,7 @@ dump_trivial_transaction(const char *vconn_name, uint8_t request_type)
 static void
 dump_stats_transaction(const char *vconn_name, struct ofpbuf *request)
 {
-    uint32_t send_xid = ((struct ofp_header *) request->data)->xid;
+    ovs_be32 send_xid = ((struct ofp_header *) request->data)->xid;
     struct vconn *vconn;
     bool done = false;
 
@@ -755,7 +755,7 @@ do_ping(int argc, char *argv[])
             ofp_print(stdout, reply, reply->size, 2);
         }
         printf("%zu bytes from %s: xid=%08"PRIx32" time=%.1f ms\n",
-               reply->size - sizeof *rpy_hdr, argv[1], rpy_hdr->xid,
+               reply->size - sizeof *rpy_hdr, argv[1], ntohl(rpy_hdr->xid),
                    (1000*(double)(end.tv_sec - start.tv_sec))
                    + (.001*(end.tv_usec - start.tv_usec)));
         ofpbuf_delete(request);