From 44381c1b4e01d051a8cc9ffccb8c37a0c2d4c808 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Tue, 16 Nov 2010 11:00:25 -0800 Subject: [PATCH] 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. --- lib/ofp-print.c | 2 +- lib/ofp-util.c | 14 +++++++------- lib/ofp-util.h | 7 ++++--- lib/vconn.c | 3 ++- utilities/ovs-ofctl.c | 4 ++-- 5 files changed, 16 insertions(+), 14 deletions(-) diff --git a/lib/ofp-print.c b/lib/ofp-print.c index 4572db44..507ed7db 100644 --- 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 8a9a8ec8..5c4336a8 100644 --- 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 3467366e..05d6acec 100644 --- a/lib/ofp-util.h +++ b/lib/ofp-util.h @@ -22,6 +22,7 @@ #include #include #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 d2a3829f..380e374a 100644 --- 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 e46e1a2d..be68bdcb 100644 --- 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); -- 2.30.2