[ovs-dev] [ofproto tests 04/29] ofp-util: Consistently treat OpenFlow xids as network byte order.
Justin Pettit
jpettit at nicira.com
Thu Nov 18 01:07:11 UTC 2010
Ah-ha. I see you decided to go back in time to address the issue I mentioned in the previous commit to look less foolish. Very clever.
I'm a bit concerned about whether users will expect the xid to be in network- or host-byte order when it's printed. On the one hand, it's really opaque data, on the other, we are "storing" some information there. The approach you've chosen is probably fine, but I worry about how it will interact with other OpenFlow protocol dissectors.
--Justin
On Nov 16, 2010, at 11:20 AM, Ben Pfaff wrote:
> 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 4572db4..507ed7d 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 8a9a8ec..5c4336a 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 3467366..05d6ace 100644
> --- 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
> --- 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
> --- 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);
> --
> 1.7.1
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
More information about the dev
mailing list