[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