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 4572db4440d55589e8fb3dbdd178201076d0c21a..507ed7db35c8751a951def7e33284604f45a0692 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 8a9a8ec8414d53761d4042f47849dbcdc1b33fb1..5c4336a812feadadce42cef4e1feb6918ea06fb8 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 3467366efc1671fa6abd41b2e8e74b2a9404cb9b..05d6acecf48ae28531aad26a28be0c7a4727cb14 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 d2a3829f347bb1bf1f1f0016d6c9d59bd54cc35c..380e374aa8b60883060f2908c6237e74da038c5a 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 e46e1a2dbe23f0ccab8d7beeb40f7fbe692e7a5e..be68bdcb1692eff78b58c56738e8663cbfcbfa67 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);