With a minor function naming question below:
Acked-by: Jarno Rajahalme <ja...@ovn.org>
> On Jan 18, 2016, at 11:26 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> At first glance, OF1.4 queue properties look a lot like those for OF1.0
> to OF1.3, but in fact their different padding makes them incompatible. In
> addition, OF1.4 switches from using regular OpenFlow messages to request
> queue properties, to using multipart messages. Thus, we really need to
> use separate code to deal with OF1.4 queues.
>
> OF1.0, OF1.1, and OF1.2 all have slightly different queue config reply
> messages, but only OF1.0 and OF1.2 had tests, so this adds tests. (There
> is no test for OF1.3 because it's the same as OF1.2.)
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---
> include/openflow/openflow-1.0.h | 37 ++++++++++++++++++++++++++++++++++++-
> include/openflow/openflow-common.h | 23 -----------------------
> lib/ofp-util.c | 27 ++++++++++++++-------------
> tests/ofp-print.at | 15 +++++++++++++++
> tests/ofproto.at | 14 ++++++++++++++
> 5 files changed, 79 insertions(+), 37 deletions(-)
>
> diff --git a/include/openflow/openflow-1.0.h b/include/openflow/openflow-1.0.h
> index 54b15f2..db629f7 100644
> --- a/include/openflow/openflow-1.0.h
> +++ b/include/openflow/openflow-1.0.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -137,6 +137,41 @@ struct ofp10_packet_queue {
> };
> OFP_ASSERT(sizeof(struct ofp10_packet_queue) == 8);
>
> +/* Queue properties for OF1.0 to OF1.3.
> + *
> + * OF1.4+ use the same numbers but rename them and change the property
> formats
> + * in incompatible ways, so there's not much benefit to sharing the names. */
> +enum ofp10_queue_properties {
> + /* Introduced in OF1.0. */
> + OFPQT10_MIN_RATE = 1, /* Minimum datarate guaranteed. */
> +
> + /* Introduced in OF1.1. */
> + OFPQT11_MAX_RATE = 2, /* Maximum guaranteed rate. */
> + OFPQT11_EXPERIMENTER = 0xffff, /* Experimenter defined property. */
> +};
> +
> +/* Description for a queue in OpenFlow 1.0 to 1.3.
> + *
> + * OF1.4+ also use a TLV format but an incompatible one. */
> +struct ofp10_queue_prop_header {
> + ovs_be16 property; /* One of OFPQT*. */
> + ovs_be16 len; /* Length of property, including this header. */
> + uint8_t pad[4]; /* 64-bit alignemnt. */
> +};
> +OFP_ASSERT(sizeof(struct ofp10_queue_prop_header) == 8);
> +
> +/* Min-Rate and Max-Rate queue property description (OFPQT10_MIN and
> + * OFPQT11_MAX).
> + *
> + * OF1.4+ use similar TLVs but they are incompatible due to different
> padding.
> + */
> +struct ofp10_queue_prop_rate {
> + struct ofp10_queue_prop_header prop_header;
> + ovs_be16 rate; /* In 1/10 of a percent; >1000 -> disabled. */
> + uint8_t pad[6]; /* 64-bit alignment */
> +};
> +OFP_ASSERT(sizeof(struct ofp10_queue_prop_rate) == 16);
> +
> /* Query for port queue configuration. */
> struct ofp10_queue_get_config_request {
> ovs_be16 port; /* Port to be queried. Should refer
> diff --git a/include/openflow/openflow-common.h
> b/include/openflow/openflow-common.h
> index 81f9120..f152718 100644
> --- a/include/openflow/openflow-common.h
> +++ b/include/openflow/openflow-common.h
> @@ -209,29 +209,6 @@ enum ofp_port_features {
> OFPPF_10GB_FD = 1 << 6, /* 10 Gb full-duplex rate support. */
> };
>
> -enum ofp_queue_properties {
> - OFPQT_MIN_RATE = 1, /* Minimum datarate guaranteed. */
> - OFPQT_MAX_RATE = 2, /* Maximum guaranteed rate. */
> - OFPQT_EXPERIMENTER = 0xffff, /* Experimenter defined property. */
> -};
> -
> -/* Common description for a queue. */
> -struct ofp_queue_prop_header {
> - ovs_be16 property; /* One of OFPQT_. */
> - ovs_be16 len; /* Length of property, including this header. */
> - uint8_t pad[4]; /* 64-bit alignemnt. */
> -};
> -OFP_ASSERT(sizeof(struct ofp_queue_prop_header) == 8);
> -
> -/* Min-Rate and Max-Rate queue property description (OFPQT_MIN and
> - * OFPQT_MAX). */
> -struct ofp_queue_prop_rate {
> - struct ofp_queue_prop_header prop_header;
> - ovs_be16 rate; /* In 1/10 of a percent; >1000 -> disabled. */
> - uint8_t pad[6]; /* 64-bit alignment */
> -};
> -OFP_ASSERT(sizeof(struct ofp_queue_prop_rate) == 16);
> -
> /* Switch features. */
> struct ofp_switch_features {
> ovs_be64 datapath_id; /* Datapath unique ID. The lower 48-bits are for
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index effd96a..c1cbcea 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -2554,11 +2554,11 @@ ofputil_encode_queue_get_config_reply(const struct
> ofp_header *oh)
> }
>
> static void
> -put_queue_rate(struct ofpbuf *reply, enum ofp_queue_properties property,
> - uint16_t rate)
> +put_ofp11_queue_rate(struct ofpbuf *reply,
> + enum ofp10_queue_properties property, uint16_t rate)
It is unclear to me why this function would not be called
‘put_ofp10_queue_rate’ instead?
> {
> if (rate != UINT16_MAX) {
> - struct ofp_queue_prop_rate *oqpr;
> + struct ofp10_queue_prop_rate *oqpr;
>
> oqpr = ofpbuf_put_zeros(reply, sizeof *oqpr);
> oqpr->prop_header.property = htons(property);
> @@ -2593,8 +2593,8 @@ ofputil_append_queue_get_config_reply(struct ofpbuf
> *reply,
> len_ofs = (char *) &opq12->len - (char *) reply->data;
> }
>
> - put_queue_rate(reply, OFPQT_MIN_RATE, oqc->min_rate);
> - put_queue_rate(reply, OFPQT_MAX_RATE, oqc->max_rate);
> + put_ofp11_queue_rate(reply, OFPQT10_MIN_RATE, oqc->min_rate);
> + put_ofp11_queue_rate(reply, OFPQT11_MAX_RATE, oqc->max_rate);
>
> len = ofpbuf_at(reply, len_ofs, sizeof *len);
> *len = htons(reply->size - start_ofs);
> @@ -2628,12 +2628,13 @@ ofputil_decode_queue_get_config_reply(struct ofpbuf
> *reply, ofp_port_t *port)
> }
>
> static enum ofperr
> -parse_queue_rate(const struct ofp_queue_prop_header *hdr, uint16_t *rate)
> +parse_ofp11_queue_rate(const struct ofp10_queue_prop_header *hdr,
> + uint16_t *rate)
Same here.
> {
> - const struct ofp_queue_prop_rate *oqpr;
> + const struct ofp10_queue_prop_rate *oqpr;
>
> if (hdr->len == htons(sizeof *oqpr)) {
> - oqpr = (const struct ofp_queue_prop_rate *) hdr;
> + oqpr = (const struct ofp10_queue_prop_rate *) hdr;
> *rate = ntohs(oqpr->rate);
> return 0;
> } else {
> @@ -2692,7 +2693,7 @@ ofputil_pull_queue_get_config_reply(struct ofpbuf
> *reply,
> len -= opq_len;
>
> while (len > 0) {
> - const struct ofp_queue_prop_header *hdr;
> + const struct ofp10_queue_prop_header *hdr;
> unsigned int property;
> unsigned int prop_len;
> enum ofperr error = 0;
> @@ -2705,12 +2706,12 @@ ofputil_pull_queue_get_config_reply(struct ofpbuf
> *reply,
>
> property = ntohs(hdr->property);
> switch (property) {
> - case OFPQT_MIN_RATE:
> - error = parse_queue_rate(hdr, &queue->min_rate);
> + case OFPQT10_MIN_RATE:
> + error = parse_ofp11_queue_rate(hdr, &queue->min_rate);
> break;
>
> - case OFPQT_MAX_RATE:
> - error = parse_queue_rate(hdr, &queue->max_rate);
> + case OFPQT11_MAX_RATE:
> + error = parse_ofp11_queue_rate(hdr, &queue->max_rate);
> break;
>
> default:
> diff --git a/tests/ofp-print.at b/tests/ofp-print.at
> index 6fae7f0..24e602e 100644
> --- a/tests/ofp-print.at
> +++ b/tests/ofp-print.at
> @@ -2561,6 +2561,21 @@ queue 17476:
> ])
> AT_CLEANUP
>
> +AT_SETUP([OFPT_QUEUE_GET_CONFIG_REPLY - OF1.1])
> +AT_KEYWORDS([ofp-print])
> +AT_CHECK([ovs-ofctl ofp-print "02 17 00 40 00 00 00 01 \
> +00 00 00 01 00 00 00 00 \
> +00 00 55 55 00 28 00 00 \
> +00 01 00 10 00 00 00 00 01 f4 00 00 00 00 00 00 \
> +00 02 00 10 00 00 00 00 02 ee 00 00 00 00 00 00 \
> +00 00 44 44 00 08 00 00 \
> +"], [0], [dnl
> +OFPT_QUEUE_GET_CONFIG_REPLY (OF1.1) (xid=0x1): port=1
> +queue 21845: min_rate:50.0% max_rate:75.0%
> +queue 17476:
> +])
> +AT_CLEANUP
> +
> AT_SETUP([OFPT_QUEUE_GET_CONFIG_REPLY - OF1.2])
> AT_KEYWORDS([ofp-print])
> AT_CHECK([ovs-ofctl ofp-print "03 17 00 50 00 00 00 01 \
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index eceff58..f29543e 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -252,6 +252,20 @@ OFPT_QUEUE_GET_CONFIG_REQUEST (xid=0x2): port=10
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto - queue configuration - (OpenFlow 1.1)])
> +OVS_VSWITCHD_START
> +ADD_OF_PORTS([br0], [1], [2])
> +AT_CHECK([ovs-ofctl -O OpenFlow12 queue-get-config br0 1], [0], [stdout])
> +AT_CHECK([STRIP_XIDS stdout], [0], [dnl
> +OFPT_QUEUE_GET_CONFIG_REPLY (OF1.2): port=1
> +])
> +AT_CHECK([ovs-ofctl -O OpenFlow12 queue-get-config br0 10], [0],
> + [OFPT_ERROR (OF1.2) (xid=0x2): OFPQOFC_BAD_PORT
> +OFPT_QUEUE_GET_CONFIG_REQUEST (OF1.2) (xid=0x2): port=10
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> AT_SETUP([ofproto - queue configuration - (OpenFlow 1.2)])
> OVS_VSWITCHD_START
> ADD_OF_PORTS([br0], [1], [2])
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
