[ovs-dev] [PATCH 1/2] Fix handling of OFPP_ANY in OpenFlow 1.1 and later.

Gurucharan Shetty shettyg at nicira.com
Thu Jan 3 18:42:59 UTC 2013


On Mon, Nov 26, 2012 at 10:11 AM, Ben Pfaff <blp at nicira.com> wrote:

> From: Jarno Rajahalme <jarno.rajahalme at 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.
>

According to manpage of ovs-ofctl, "The string * or ANY may be specified to
explicitly mark any of  these fields as a wildcard."

If in_port=ANY, it is treated as a wildcard now. Is this okay? (NONE is now
replaced by ANY in ofctl prints)

The definition of OFPP_NONE states "Not associated with a physical port.".
I am not sure it is the same as a wildcard.

Thanks,
Guru



> ---
> 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.
>
>  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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130103/b10cc22e/attachment-0003.html>


More information about the dev mailing list