[ovs-dev] [PATCH OF12+ 1/4 v2] OpenFlow 1.2 fixes and initial OpenFlow 1.3 support
Jarno Rajahalme
jarno.rajahalme at nsn.com
Fri Nov 23 11:33:55 UTC 2012
On Nov 23, 2012, at 2:14 , ext Ben Pfaff wrote:
> On Thu, Nov 22, 2012 at 05:24:06PM +0200, Jarno Rajahalme wrote:
>> OpenFlow 1.2 protocol fixes: Add OFPP_ANY to include/openflow/openflow-1.1.h,
>> and allow it as a port in queue stats request. Make ovs_ofctl use OFPP_ANY
>> instead of OFPP_ALL for queue stats requests on OF 1.1+.
>> Do not check out_group on flow_mod unless the command is DELETE*.
>
> Thank you for this patch (and the rest)!
>
> The change to ofputil_decode_flow_mod() looks good, except that you
> should break the line so that it does not exceed 79 columns wide.
>
> The purpose of ofputil_queue_stats_request and the functions that encode
> and decode it is to insulate other code from having to know the
> differences between the various OpenFlow versions. Therefore, instead
> of modifying handle_queue_stats_request() and ofctl_queue_stats() to
> support the various versions, I would modify
> ofputil_encode_queue_stats_request() and
> ofputil_decode_queue_stats_request() so that they do not have to know
> the difference. For example, ofputil_encode_queue_stats_request() could
> translate OFPP_ANY to the appropriate value in whatever version it is
> encoding, and ofputil_decode_queue_stats_request() could translate the
> particular version's value to OFPP_ANY. The clients would then use
> OFPP_ANY regardless of version.
Below is an updated patch to do the above.
OF 1.1+ uses OFPP_ANY also for flow_mod, flow_stats, and port_stats requests. ofp-util.c now uses OFPP_NONE for that purpose, which is the same and thus works fine. Semantically, however, OFPP_ANY would be a better fit for the function for selecting all ports than OFPP_NONE. Therefore it might be good to update ofp_util.c to use OFPP_ANY instead of OFPP_NONE for selecting all ports. As it relates to this patch that adds the definition of OFPP_ANY, it might be a good idea to do that change while we are at it. thoughts?
diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h
index 9785db4..c4a5aba 100644
--- a/include/openflow/openflow-1.1.h
+++ b/include/openflow/openflow-1.1.h
@@ -70,6 +70,14 @@
#define OFPP11_MAX 0xffffff00
#define OFPP11_OFFSET (OFPP11_MAX - OFPP_MAX)
+/* Reserved wildcard port used only for flow mod (delete) and flow stats
+ * requests. Selects all flows regardless of output port
+ * (including flows with no output port)
+ *
+ * Define it via OFPP_NONE (0xFFFF) so that OFPP_ANY is still an enum ofp_port
+ */
+#define OFPP_ANY OFPP_NONE
+
/* OpenFlow 1.1 port config flags are just the common flags. */
#define OFPPC11_ALL \
(OFPPC_PORT_DOWN | OFPPC_NO_RECV | OFPPC_NO_FWD | OFPPC_NO_PACKET_IN)
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 4facf0a..0664945 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1435,7 +1435,9 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
if (error) {
return error;
}
- if (ofm->out_group != htonl(OFPG_ANY)) {
+ if ((ofm->command == OFPFC_DELETE
+ || ofm->command == OFPFC_DELETE_STRICT)
+ && ofm->out_group != htonl(OFPG_ANY)) {
return OFPERR_OFPFMFC_UNKNOWN;
}
fm->flags = ntohs(ofm->flags);
@@ -4467,9 +4469,12 @@ ofputil_decode_queue_stats_request(const struct ofp_header *request,
}
case OFP10_VERSION: {
- const struct ofp10_queue_stats_request *qsr11 = ofpmsg_body(request);
- oqsr->queue_id = ntohl(qsr11->queue_id);
- oqsr->port_no = ntohs(qsr11->port_no);
+ const struct ofp10_queue_stats_request *qsr10 = ofpmsg_body(request);
+ oqsr->queue_id = ntohl(qsr10->queue_id);
+ oqsr->port_no = ntohs(qsr10->port_no);
+ /* OF 1.0 uses OFPP_ALL for OFPP_ANY */
+ if (oqsr->port_no == OFPP_ALL)
+ oqsr->port_no = OFPP_ANY;
return 0;
}
@@ -4501,7 +4506,9 @@ ofputil_encode_queue_stats_request(enum ofp_version ofp_version,
struct ofp10_queue_stats_request *req;
request = ofpraw_alloc(OFPRAW_OFPST10_QUEUE_REQUEST, ofp_version, 0);
req = ofpbuf_put_zeros(request, sizeof *req);
- req->port_no = htons(oqsr->port_no);
+ /* OpenFlow 1.0 needs OFPP_ALL instead of OFPP_ANY */
+ req->port_no = htons(oqsr->port_no == OFPP_ANY
+ ? OFPP_ALL : oqsr->port_no);
req->queue_id = htonl(oqsr->queue_id);
break;
}
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index dabb590..5721dad 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3054,7 +3054,7 @@ handle_queue_stats_request(struct ofconn *ofconn,
return error;
}
- if (oqsr.port_no == OFPP_ALL) {
+ if (oqsr.port_no == OFPP_ANY) {
error = OFPERR_OFPQOFC_BAD_QUEUE;
HMAP_FOR_EACH (port, hmap_node, &ofproto->ports) {
if (!handle_queue_stats_for_port(port, oqsr.queue_id, &cbdata)) {
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 363c0a3..5fc778e 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -968,7 +968,7 @@ ofctl_queue_stats(int argc, char *argv[])
if (argc > 2 && argv[2][0] && strcasecmp(argv[2], "all")) {
oqs.port_no = str_to_port_no(argv[1], argv[2]);
} else {
- oqs.port_no = OFPP_ALL;
+ oqs.port_no = OFPP_ANY;
}
if (argc > 3 && argv[3][0] && strcasecmp(argv[3], "all")) {
oqs.queue_id = atoi(argv[3]);
More information about the dev
mailing list