On Nov 26, 2012, at 20:11 , ext Ben Pfaff wrote:
> From: Jarno Rajahalme <jarno.rajaha...@nsn.com>
>
> 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+.
>
> This patch changes "none" ports print out. "none" is still accepted on input
> for backwards compatibility, but it prints out as "ANY". To make this less
> confusing, I changed the test cases to use "controller" or "any" instead of
> "none". The test case that tests for both "none" and "controller" still tests
> for them.
> ---
> This is most of patch 1/4 that you sent out earlier. I've added a
> comment on struct ofputil_queue_stats_request to made it clear that
> OFPP_ANY is to be used there.
>
> Before I can commit this or any of the other patches, I need a
> Signed-off-by: line from you. There is information on the form
> and meaning of this line in SubmittingPatches at the top of the
> source tree. Can you provide it? Thanks.
Here is the Signed-off-by line for this patch:
Signed-off-by: Jarno Rajahalme <jarno.rajaha...@nsn.com>
>
> include/openflow/openflow-1.1.h | 8 ++++++++
> lib/ofp-parse.c | 2 +-
> lib/ofp-print.c | 4 ++--
> lib/ofp-util.c | 21 ++++++++++++++++-----
> lib/ofp-util.h | 2 +-
> ofproto/ofproto.c | 14 +++++++-------
> tests/ofp-print.at | 10 +++++-----
> tests/ofproto.at | 22 +++++++++++-----------
> utilities/ovs-ofctl.c | 8 ++++----
> 9 files changed, 55 insertions(+), 36 deletions(-)
>
> 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-parse.c b/lib/ofp-parse.c
> index 054db60..1d71370 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -829,7 +829,7 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command,
> const char *str_,
> fm->idle_timeout = OFP_FLOW_PERMANENT;
> fm->hard_timeout = OFP_FLOW_PERMANENT;
> fm->buffer_id = UINT32_MAX;
> - fm->out_port = OFPP_NONE;
> + fm->out_port = OFPP_ANY;
> fm->flags = 0;
> if (fields & F_ACTIONS) {
> act_str = strstr(string, "action");
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index a0f04dc..1e35fba 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -765,7 +765,7 @@ ofp_print_flow_mod(struct ds *s, const struct ofp_header
> *oh, int verbosity)
> if (fm.buffer_id != UINT32_MAX) {
> ds_put_format(s, "buf:0x%"PRIx32" ", fm.buffer_id);
> }
> - if (fm.out_port != OFPP_NONE) {
> + if (fm.out_port != OFPP_ANY) {
> ds_put_format(s, "out_port:");
> ofputil_format_port(fm.out_port, s);
> ds_put_char(s, ' ');
> @@ -1003,7 +1003,7 @@ ofp_print_flow_stats_request(struct ds *string, const
> struct ofp_header *oh)
> ds_put_format(string, " table=%"PRIu8, fsr.table_id);
> }
>
> - if (fsr.out_port != OFPP_NONE) {
> + if (fsr.out_port != OFPP_ANY) {
> ds_put_cstr(string, " out_port=");
> ofputil_format_port(fsr.out_port, string);
> }
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 4facf0a..b7feff8 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -3804,6 +3804,11 @@ ofputil_check_output_port(uint16_t port, int max_ports)
> OFPUTIL_NAMED_PORT(ALL) \
> OFPUTIL_NAMED_PORT(CONTROLLER) \
> OFPUTIL_NAMED_PORT(LOCAL) \
> + OFPUTIL_NAMED_PORT(ANY)
> +
> +/* For backwards compatibility, so that "none" is recognized as OFPP_ANY */
> +#define OFPUTIL_NAMED_PORTS_WITH_NONE \
> + OFPUTIL_NAMED_PORTS \
> OFPUTIL_NAMED_PORT(NONE)
>
> /* Stores the port number represented by 's' into '*portp'. 's' may be an
> @@ -3863,7 +3868,7 @@ ofputil_port_from_string(const char *s, uint16_t *portp)
> };
> static const struct pair pairs[] = {
> #define OFPUTIL_NAMED_PORT(NAME) {#NAME, OFPP_##NAME},
> - OFPUTIL_NAMED_PORTS
> + OFPUTIL_NAMED_PORTS_WITH_NONE
> #undef OFPUTIL_NAMED_PORT
> };
> const struct pair *p;
> @@ -4467,9 +4472,13 @@ 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 +4510,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/lib/ofp-util.h b/lib/ofp-util.h
> index 053cd84..ed1a538 100644
> --- a/lib/ofp-util.h
> +++ b/lib/ofp-util.h
> @@ -686,7 +686,7 @@ enum ofperr ofputil_decode_port_stats_request(const
> struct ofp_header *request,
> uint16_t *ofp10_port);
>
> struct ofputil_queue_stats_request {
> - uint16_t port_no;
> + uint16_t port_no; /* OFPP_ANY means "all ports". */
> uint32_t queue_id;
> };
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index dabb590..0992fe4 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2129,7 +2129,7 @@ ofproto_rule_destroy(struct rule *rule)
> bool
> ofproto_rule_has_out_port(const struct rule *rule, uint16_t port)
> {
> - return (port == OFPP_NONE
> + return (port == OFPP_ANY
> || ofpacts_output_to_port(rule->ofpacts, rule->ofpacts_len,
> port));
> }
>
> @@ -2542,7 +2542,7 @@ handle_port_stats_request(struct ofconn *ofconn,
> }
>
> ofpmp_init(&replies, request);
> - if (port_no != OFPP_NONE) {
> + if (port_no != OFPP_ANY) {
> port = ofproto_get_port(p, port_no);
> if (port) {
> append_port_stat(port, &replies);
> @@ -2659,7 +2659,7 @@ next_matching_table(const struct ofproto *ofproto,
> * OpenFlow OFPFC_MODIFY and OFPFC_DELETE requests and puts them on list
> * 'rules'.
> *
> - * If 'out_port' is anything other than OFPP_NONE, then only rules that
> output
> + * If 'out_port' is anything other than OFPP_ANY, then only rules that output
> * to 'out_port' are included.
> *
> * Hidden rules are always omitted.
> @@ -2710,7 +2710,7 @@ exit:
> * OpenFlow OFPFC_MODIFY_STRICT and OFPFC_DELETE_STRICT requests and puts them
> * on list 'rules'.
> *
> - * If 'out_port' is anything other than OFPP_NONE, then only rules that
> output
> + * If 'out_port' is anything other than OFPP_ANY, then only rules that output
> * to 'out_port' are included.
> *
> * Hidden rules are always omitted.
> @@ -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)) {
> @@ -3327,7 +3327,7 @@ modify_flows_loose(struct ofproto *ofproto, struct
> ofconn *ofconn,
>
> error = collect_rules_loose(ofproto, fm->table_id, &fm->match,
> fm->cookie, fm->cookie_mask,
> - OFPP_NONE, &rules);
> + OFPP_ANY, &rules);
> if (error) {
> return error;
> } else if (list_is_empty(&rules)) {
> @@ -3352,7 +3352,7 @@ modify_flow_strict(struct ofproto *ofproto, struct
> ofconn *ofconn,
>
> error = collect_rules_strict(ofproto, fm->table_id, &fm->match,
> fm->priority, fm->cookie, fm->cookie_mask,
> - OFPP_NONE, &rules);
> + OFPP_ANY, &rules);
>
> if (error) {
> return error;
> diff --git a/tests/ofp-print.at b/tests/ofp-print.at
> index bcf9d2c..7869a86 100644
> --- a/tests/ofp-print.at
> +++ b/tests/ofp-print.at
> @@ -1149,7 +1149,7 @@ AT_CHECK([ovs-ofctl ofp-print "\
> 01 10 00 14 00 00 00 01 00 05 00 00 ff fc 00 00 \
> ff ff ff ff \
> "], [0], [dnl
> -OFPST_QUEUE request (xid=0x1):port=ALL queue=ALL
> +OFPST_QUEUE request (xid=0x1):port=ANY queue=ALL
> ])
> AT_CLEANUP
>
> @@ -1157,9 +1157,9 @@ AT_SETUP([OFPST_QUEUE request - OF1.1])
> AT_KEYWORDS([ofp-print OFPT_STATS_REQUEST])
> AT_CHECK([ovs-ofctl ofp-print "\
> 02 12 00 18 00 00 00 02 00 05 00 00 00 00 00 00 \
> -ff ff ff fc ff ff ff ff \
> +ff ff ff ff ff ff ff ff \
> "], [0], [dnl
> -OFPST_QUEUE request (OF1.1) (xid=0x2):port=ALL queue=ALL
> +OFPST_QUEUE request (OF1.1) (xid=0x2):port=ANY queue=ALL
> ])
> AT_CLEANUP
>
> @@ -1167,9 +1167,9 @@ AT_SETUP([OFPST_QUEUE request - OF1.2])
> AT_KEYWORDS([ofp-print OFPT_STATS_REQUEST])
> AT_CHECK([ovs-ofctl ofp-print "\
> 03 12 00 18 00 00 00 02 00 05 00 00 00 00 00 00 \
> -ff ff ff fc ff ff ff ff \
> +ff ff ff ff ff ff ff ff \
> "], [0], [dnl
> -OFPST_QUEUE request (OF1.2) (xid=0x2):port=ALL queue=ALL
> +OFPST_QUEUE request (OF1.2) (xid=0x2):port=ANY queue=ALL
> ])
> AT_CLEANUP
>
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 38bc406..7f468bb 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -90,9 +90,9 @@ AT_CHECK([ovs-ofctl -vwarn queue-stats br0], [0], [stdout])
> AT_CHECK([STRIP_XIDS stdout], [0], [dnl
> OFPST_QUEUE reply: 0 queues
> ])
> -AT_CHECK([ovs-ofctl -vwarn queue-stats br0 ALL 5], [0],
> +AT_CHECK([ovs-ofctl -vwarn queue-stats br0 ANY 5], [0],
> [OFPT_ERROR (xid=0x2): OFPQOFC_BAD_QUEUE
> -OFPST_QUEUE request (xid=0x2):port=ALL queue=5
> +OFPST_QUEUE request (xid=0x2):port=ANY queue=5
> ])
> AT_CHECK([ovs-ofctl -vwarn queue-stats br0 10], [0],
> [OFPT_ERROR (xid=0x2): OFPQOFC_BAD_PORT
> @@ -657,23 +657,23 @@ check_async () {
> : > expout
>
> # OFPT_PACKET_IN, OFPR_ACTION (controller_id=0)
> - ovs-ofctl -v packet-out br0 none controller
> '0001020304050010203040501234'
> + ovs-ofctl -v packet-out br0 controller controller
> '0001020304050010203040501234'
> if test X"$1" = X"OFPR_ACTION"; then shift;
> - echo >>expout "OFPT_PACKET_IN: total_len=14 in_port=NONE (via
> action) data_len=14 (unbuffered)
> + echo >>expout "OFPT_PACKET_IN: total_len=14 in_port=CONTROLLER (via
> action) data_len=14 (unbuffered)
> priority=0,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x1234"
> fi
>
> # OFPT_PACKET_IN, OFPR_NO_MATCH (controller_id=123)
> - ovs-ofctl -v packet-out br0 none 'controller(reason=no_match,id=123)'
> '0001020304050010203040501234'
> + ovs-ofctl -v packet-out br0 controller
> 'controller(reason=no_match,id=123)' '0001020304050010203040501234'
> if test X"$1" = X"OFPR_NO_MATCH"; then shift;
> - echo >>expout "OFPT_PACKET_IN: total_len=14 in_port=NONE (via
> no_match) data_len=14 (unbuffered)
> + echo >>expout "OFPT_PACKET_IN: total_len=14 in_port=CONTROLLER (via
> no_match) data_len=14 (unbuffered)
> priority=0,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x1234"
> fi
>
> # OFPT_PACKET_IN, OFPR_INVALID_TTL (controller_id=0)
> - ovs-ofctl packet-out br0 none dec_ttl
> '002583dfb4000026b98cb0f908004500003fb7e200000011339bac11370dac100002d7730035002b8f6d86fb0100000100000000000006626c702d7873066e696369726103636f6d00000f00'
> + ovs-ofctl packet-out br0 controller dec_ttl
> '002583dfb4000026b98cb0f908004500003fb7e200000011339bac11370dac100002d7730035002b8f6d86fb0100000100000000000006626c702d7873066e696369726103636f6d00000f00'
> if test X"$1" = X"OFPR_INVALID_TTL"; then shift;
> - echo >>expout "OFPT_PACKET_IN: total_len=76 in_port=NONE (via
> invalid_ttl) data_len=76 (unbuffered)
> + echo >>expout "OFPT_PACKET_IN: total_len=76 in_port=CONTROLLER (via
> invalid_ttl) data_len=76 (unbuffered)
> priority=0,udp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172.17.55.13,nw_dst=172.16.0.2,nw_tos=0,nw_ecn=0,nw_ttl=0,tp_src=55155,tp_dst=53
> udp_csum:8f6d"
> fi
>
> @@ -771,7 +771,7 @@ ovs-appctl -t ovs-ofctl ofctl/barrier
> ovs-appctl -t ovs-ofctl exit
>
> AT_CHECK([sed 's/ (xid=0x[[0-9a-fA-F]]*)//' monitor.log], [0], [dnl
> -OFPT_PACKET_IN: total_len=14 in_port=NONE (via action) data_len=14
> (unbuffered)
> +OFPT_PACKET_IN: total_len=14 in_port=ANY (via action) data_len=14
> (unbuffered)
> priority=0,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x1234
> OFPT_PACKET_IN: total_len=14 in_port=CONTROLLER (via action) data_len=14
> (unbuffered)
> priority=0,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x5678
> @@ -794,14 +794,14 @@ ovs-appctl -t ovs-ofctl ofctl/set-output-file
> monitor.log
> AT_CAPTURE_FILE([monitor.log])
>
> # Send a packet-out with a load action to set some metadata, and forward to
> controller
> -AT_CHECK([ovs-ofctl packet-out br0 none
> 'load(0xfafafafa5a5a5a5a->OXM_OF_METADATA[[0..63]]), controller'
> '0001020304050010203040501234'])
> +AT_CHECK([ovs-ofctl packet-out br0 controller
> 'load(0xfafafafa5a5a5a5a->OXM_OF_METADATA[[0..63]]), controller'
> '0001020304050010203040501234'])
>
> # Stop the monitor and check its output.
> ovs-appctl -t ovs-ofctl ofctl/barrier
> ovs-appctl -t ovs-ofctl exit
>
> AT_CHECK([sed 's/ (xid=0x[[0-9a-fA-F]]*)//' monitor.log], [0], [dnl
> -NXT_PACKET_IN: total_len=14 in_port=NONE metadata=0xfafafafa5a5a5a5a (via
> action) data_len=14 (unbuffered)
> +NXT_PACKET_IN: total_len=14 in_port=CONTROLLER metadata=0xfafafafa5a5a5a5a
> (via action) data_len=14 (unbuffered)
> priority=0,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x1234
> OFPT_BARRIER_REPLY:
> ])
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 363c0a3..5037398 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]);
> @@ -1430,7 +1430,7 @@ ofctl_dump_ports(int argc, char *argv[])
> uint16_t port;
>
> open_vconn(argv[1], &vconn);
> - port = argc > 2 ? str_to_port_no(argv[1], argv[2]) : OFPP_NONE;
> + port = argc > 2 ? str_to_port_no(argv[1], argv[2]) : OFPP_ANY;
> request = ofputil_encode_dump_ports_request(vconn_get_version(vconn),
> port);
> dump_stats_transaction(vconn, request);
> vconn_close(vconn);
> @@ -1940,7 +1940,7 @@ read_flows_from_switch(struct vconn *vconn,
>
> fsr.aggregate = false;
> match_init_catchall(&fsr.match);
> - fsr.out_port = OFPP_NONE;
> + fsr.out_port = OFPP_ANY;
> fsr.table_id = 0xff;
> fsr.cookie = fsr.cookie_mask = htonll(0);
> request = ofputil_encode_flow_stats_request(&fsr, protocol);
> @@ -1983,7 +1983,7 @@ fte_make_flow_mod(const struct fte *fte, int index,
> uint16_t command,
> fm.idle_timeout = version->idle_timeout;
> fm.hard_timeout = version->hard_timeout;
> fm.buffer_id = UINT32_MAX;
> - fm.out_port = OFPP_NONE;
> + fm.out_port = OFPP_ANY;
> fm.flags = version->flags;
> if (command == OFPFC_ADD || command == OFPFC_MODIFY ||
> command == OFPFC_MODIFY_STRICT) {
> --
> 1.7.2.5
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
