There's some trailing whitespace in this patch.
s/consisten/consistent/
Thanks for adding unit tests. Does it make sense to add one that
sorts by multiple fields specified on the command line? Perhaps adding
a sort by in_port to the test which does tp_src would give us slightly
better coverage.
Is there any particular reason not to always sort based on priority
even if neither sort nor rsort are specified? I suppose it's slightly
less efficient, but I'm not sure that matters in particular. I think
it would cleanup the code every so slightly because we wouldn't have
to special case n_criteria = 0.
Looks good thanks,
Ethan
On Tue, Jul 3, 2012 at 2:48 PM, Ben Pfaff <b...@nicira.com> wrote:
> Signed-off-by: Arun Sharma <arun.sha...@calsoftinc.com>
> [b...@nicira.com rewrote most of the code]
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> NEWS | 1 +
> lib/ofp-print.c | 80 +++++++++++++---------
> lib/ofp-print.h | 6 ++
> tests/ovs-ofctl.at | 77 +++++++++++++++++++++
> utilities/ovs-ofctl.8.in | 33 +++++++++
> utilities/ovs-ofctl.c | 168
> ++++++++++++++++++++++++++++++++++++++++++++--
> 6 files changed, 326 insertions(+), 39 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 1966ac2..cbd9784 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -5,6 +5,7 @@ post-v1.7.0
> - A new test utility that can create L3 tunnel between two Open
> vSwitches and detect connectivity issues.
> - ovs-ofctl:
> + - New --sort and --rsort options for "dump-flows" command.
> - "mod-port" command can now control all OpenFlow config flags.
> - OpenFlow:
> - Allow general bitwise masking for IPv4 and IPv6 addresses in
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index 5103c3e..d1faac9 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -1223,6 +1223,37 @@ ofp_print_flow_stats_request(struct ds *string,
> cls_rule_format(&fsr.match, string);
> }
>
> +void
> +ofp_print_flow_stats(struct ds *string, struct ofputil_flow_stats *fs)
> +{
> + ds_put_format(string, " cookie=0x%"PRIx64", duration=",
> + ntohll(fs->cookie));
> +
> + ofp_print_duration(string, fs->duration_sec, fs->duration_nsec);
> + ds_put_format(string, ", table=%"PRIu8", ", fs->table_id);
> + ds_put_format(string, "n_packets=%"PRIu64", ", fs->packet_count);
> + ds_put_format(string, "n_bytes=%"PRIu64", ", fs->byte_count);
> + if (fs->idle_timeout != OFP_FLOW_PERMANENT) {
> + ds_put_format(string, "idle_timeout=%"PRIu16", ", fs->idle_timeout);
> + }
> + if (fs->hard_timeout != OFP_FLOW_PERMANENT) {
> + ds_put_format(string, "hard_timeout=%"PRIu16", ", fs->hard_timeout);
> + }
> + if (fs->idle_age >= 0) {
> + ds_put_format(string, "idle_age=%d, ", fs->idle_age);
> + }
> + if (fs->hard_age >= 0 && fs->hard_age != fs->duration_sec) {
> + ds_put_format(string, "hard_age=%d, ", fs->hard_age);
> + }
> +
> + cls_rule_format(&fs->rule, string);
> + if (string->string[string->length - 1] != ' ') {
> + ds_put_char(string, ' ');
> + }
> +
> + ofp_print_actions(string, fs->actions, fs->n_actions);
> +}
> +
> static void
> ofp_print_flow_stats_reply(struct ds *string, const struct ofp_header *oh)
> {
> @@ -1240,33 +1271,8 @@ ofp_print_flow_stats_reply(struct ds *string, const
> struct ofp_header *oh)
> }
> break;
> }
> -
> ds_put_char(string, '\n');
> -
> - ds_put_format(string, " cookie=0x%"PRIx64", duration=",
> - ntohll(fs.cookie));
> - ofp_print_duration(string, fs.duration_sec, fs.duration_nsec);
> - ds_put_format(string, ", table=%"PRIu8", ", fs.table_id);
> - ds_put_format(string, "n_packets=%"PRIu64", ", fs.packet_count);
> - ds_put_format(string, "n_bytes=%"PRIu64", ", fs.byte_count);
> - if (fs.idle_timeout != OFP_FLOW_PERMANENT) {
> - ds_put_format(string, "idle_timeout=%"PRIu16", ",
> fs.idle_timeout);
> - }
> - if (fs.hard_timeout != OFP_FLOW_PERMANENT) {
> - ds_put_format(string, "hard_timeout=%"PRIu16", ",
> fs.hard_timeout);
> - }
> - if (fs.idle_age >= 0) {
> - ds_put_format(string, "idle_age=%d, ", fs.idle_age);
> - }
> - if (fs.hard_age >= 0 && fs.hard_age != fs.duration_sec) {
> - ds_put_format(string, "hard_age=%d, ", fs.hard_age);
> - }
> -
> - cls_rule_format(&fs.rule, string);
> - if (string->string[string->length - 1] != ' ') {
> - ds_put_char(string, ' ');
> - }
> - ofp_print_actions(string, fs.actions, fs.n_actions);
> + ofp_print_flow_stats(string, &fs);
> }
> }
>
> @@ -1602,15 +1608,10 @@ ofp_print_nxt_set_controller_id(struct ds *string,
> ds_put_format(string, " id=%"PRIu16, ntohs(nci->controller_id));
> }
>
> -static void
> -ofp_to_string__(const struct ofp_header *oh,
> - const struct ofputil_msg_type *type, struct ds *string,
> - int verbosity)
> +void
> +ofp_print_version(const struct ofp_header *oh,
> + struct ds *string)
> {
> - enum ofputil_msg_code code;
> - const void *msg = oh;
> -
> - ds_put_cstr(string, ofputil_msg_type_name(type));
> switch (oh->version) {
> case OFP10_VERSION:
> break;
> @@ -1625,7 +1626,18 @@ ofp_to_string__(const struct ofp_header *oh,
> break;
> }
> ds_put_format(string, " (xid=0x%"PRIx32"):", ntohl(oh->xid));
> +}
> +
> +static void
> +ofp_to_string__(const struct ofp_header *oh,
> + const struct ofputil_msg_type *type, struct ds *string,
> + int verbosity)
> +{
> + enum ofputil_msg_code code;
> + const void *msg = oh;
>
> + ds_put_cstr(string, ofputil_msg_type_name(type));
> + ofp_print_version(oh, string);
> code = ofputil_msg_type_code(type);
> switch (code) {
> case OFPUTIL_MSG_INVALID:
> diff --git a/lib/ofp-print.h b/lib/ofp-print.h
> index ae868a4..a58235c 100644
> --- a/lib/ofp-print.h
> +++ b/lib/ofp-print.h
> @@ -26,6 +26,9 @@ struct ofp_flow_mod;
> struct ofp10_match;
> struct ds;
> union ofp_action;
> +struct ofputil_flow_stats;
> +struct ofp_header;
> +struct ofputil_msg_type;
>
> #ifdef __cplusplus
> extern "C" {
> @@ -41,6 +44,9 @@ char *ofp_to_string(const void *, size_t, int verbosity);
> char *ofp10_match_to_string(const struct ofp10_match *, int verbosity);
> char *ofp_packet_to_string(const void *data, size_t len);
>
> +void ofp_print_flow_stats(struct ds *, struct ofputil_flow_stats *);
> +void ofp_print_version(const struct ofp_header *, struct ds *);
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
> index 491e0ab..43e97c1 100644
> --- a/tests/ovs-ofctl.at
> +++ b/tests/ovs-ofctl.at
> @@ -1332,3 +1332,80 @@ ofp_util|INFO|post: @&t@
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +dnl Check that --sort and --rsort works with dump-flows
> +dnl Default field is 'priority'. Flow entries are displayed based
> +dnl on field to sort.
> +AT_SETUP([ovs-ofctl dump-flows with sorting])
> +OVS_VSWITCHD_START
> +AT_KEYWORDS([dump-flows-sort])
> +AT_DATA([allflows.txt], [[
> +priority=4,in_port=23213 actions=output:42
> +priority=5,in_port=1029 actions=output:43
> +priority=7,in_port=1029 actions=output:43
> +priority=3,in_port=1028 actions=output:44
> +priority=1,in_port=1026 actions=output:45
> +priority=6,in_port=1027 actions=output:64
> +priority=2,in_port=1025 actions=output:47
> +priority=8,tcp,tp_src=5 actions=drop
> +priority=9,tcp,tp_src=6 actions=drop
> +]])
> +
> +AT_CHECK([ovs-ofctl add-flows br0 allflows.txt
> +], [0], [ignore])
> +AT_CHECK([ovs-ofctl --sort dump-flows br0 | ofctl_strip], [0], [dnl
> + priority=1,in_port=1026 actions=output:45
> + priority=2,in_port=1025 actions=output:47
> + priority=3,in_port=1028 actions=output:44
> + priority=4,in_port=23213 actions=output:42
> + priority=5,in_port=1029 actions=output:43
> + priority=6,in_port=1027 actions=output:64
> + priority=7,in_port=1029 actions=output:43
> + priority=8,tcp,tp_src=5 actions=drop
> + priority=9,tcp,tp_src=6 actions=drop
> +])
> +AT_CHECK([ovs-ofctl --rsort dump-flows br0 | ofctl_strip], [0], [dnl
> + priority=9,tcp,tp_src=6 actions=drop
> + priority=8,tcp,tp_src=5 actions=drop
> + priority=7,in_port=1029 actions=output:43
> + priority=6,in_port=1027 actions=output:64
> + priority=5,in_port=1029 actions=output:43
> + priority=4,in_port=23213 actions=output:42
> + priority=3,in_port=1028 actions=output:44
> + priority=2,in_port=1025 actions=output:47
> + priority=1,in_port=1026 actions=output:45
> +])
> +AT_CHECK([ovs-ofctl --sort=in_port dump-flows br0 | ofctl_strip], [0], [dnl
> + priority=2,in_port=1025 actions=output:47
> + priority=1,in_port=1026 actions=output:45
> + priority=6,in_port=1027 actions=output:64
> + priority=3,in_port=1028 actions=output:44
> + priority=7,in_port=1029 actions=output:43
> + priority=5,in_port=1029 actions=output:43
> + priority=4,in_port=23213 actions=output:42
> + priority=9,tcp,tp_src=6 actions=drop
> + priority=8,tcp,tp_src=5 actions=drop
> +])
> +AT_CHECK([ovs-ofctl --rsort=in_port dump-flows br0 | ofctl_strip], [0], [dnl
> + priority=4,in_port=23213 actions=output:42
> + priority=7,in_port=1029 actions=output:43
> + priority=5,in_port=1029 actions=output:43
> + priority=3,in_port=1028 actions=output:44
> + priority=6,in_port=1027 actions=output:64
> + priority=1,in_port=1026 actions=output:45
> + priority=2,in_port=1025 actions=output:47
> + priority=9,tcp,tp_src=6 actions=drop
> + priority=8,tcp,tp_src=5 actions=drop
> +])
> +AT_CHECK([ovs-ofctl --sort=tcp_src dump-flows br0 | ofctl_strip], [0], [dnl
> + priority=8,tcp,tp_src=5 actions=drop
> + priority=9,tcp,tp_src=6 actions=drop
> + priority=7,in_port=1029 actions=output:43
> + priority=6,in_port=1027 actions=output:64
> + priority=5,in_port=1029 actions=output:43
> + priority=4,in_port=23213 actions=output:42
> + priority=3,in_port=1028 actions=output:44
> + priority=2,in_port=1025 actions=output:47
> + priority=1,in_port=1026 actions=output:45
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> index 9c4ea0c..53d3848 100644
> --- a/utilities/ovs-ofctl.8.in
> +++ b/utilities/ovs-ofctl.8.in
> @@ -164,6 +164,12 @@ in the switch are retrieved. See \fBFlow Syntax\fR,
> below, for the
> syntax of \fIflows\fR. The output format is described in
> \fBTable Entry Output\fR.
> .
> +.IP
> +By default, \fBovs\-ofctl\fR prints flow entries in the same order
> +that the switch sends them, which is unlikely to be intuitive or
> +consisten. See the description of \fB\-\-sort\fR and \fB\-\-rsort\fR,
> +under \fBOPTIONS\fR below, to influence the display order.
> +.
> .TP
> \fBdump\-aggregate \fIswitch \fR[\fIflows\fR]
> Prints to the console aggregate statistics for flows in
> @@ -1317,6 +1323,33 @@ Increases the verbosity of OpenFlow messages printed
> and logged by
> \fBovs\-ofctl\fR commands. Specify this option more than once to
> increase verbosity further.
> .
> +.IP \fB\-\-sort\fR[\fB=\fIfield\fR]
> +.IQ \fB\-\-rsort\fR[\fB=\fIfield\fR]
> +Display output sorted by flow \fIfield\fR in ascending
> +(\fB\-\-sort\fR) or descending (\fB\-\-rsort\fR) order, where
> +\fIfield\fR is any of the fields that are allowed for matching or
> +\fBpriority\fR to sort by priority. When \fIfield\fR is omitted, the
> +output is sorted by priority. Specify these options multiple times to
> +sort by multiple fields.
> +.IP
> +Any given flow will not necessarily specify a value for a given
> +field. This requires special treatement:
> +.RS
> +.IP \(bu
> +A flow that does not specify any part of a field that is used for sorting is
> +sorted after all the flows that do specify the field. For example,
> +\fB\-\-sort=tcp_src\fR will sort all the flows that specify a TCP
> +source port in ascending order, followed by the flows that do not
> +specify a TCP source port at all.
> +.IP \(bu
> +A flow that only specifies some bits in a field is sorted as if the
> +wildcarded bits were zero. For example, \fB\-\-sort=nw_src\fR would
> +sort a flow that specifies \fBnw_src=192.168.0.0/24\fR the same as
> +\fBnw_src=192.168.0.0\fR.
> +.RE
> +.IP
> +These options currently affect only \fBdump\-flows\fR output.
> +.
> .ds DD \
> \fBovs\-ofctl\fR detaches only when executing the \fBmonitor\fR or \
> \fBsnoop\fR commands.
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 2f7a1bb..8d2ac0e 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -55,6 +55,8 @@
> #include "util.h"
> #include "vconn.h"
> #include "vlog.h"
> +#include "meta-flow.h"
> +#include "sort.h"
>
> VLOG_DEFINE_THIS_MODULE(ofctl);
>
> @@ -83,11 +85,23 @@ static int verbosity;
> * "snoop" command? */
> static bool timestamp;
>
> +/* --sort, --rsort: Sort order. */
> +enum sort_order { SORT_ASC, SORT_DESC };
> +struct sort_criterion {
> + const struct mf_field *field; /* NULL means to sort by priority. */
> + enum sort_order order;
> +};
> +static struct sort_criterion *criteria;
> +static size_t n_criteria, allocated_criteria;
> +
> static const struct command all_commands[];
>
> static void usage(void) NO_RETURN;
> static void parse_options(int argc, char *argv[]);
>
> +static bool recv_flow_stats_reply(struct vconn *, ovs_be32 send_xid,
> + struct ofpbuf **replyp,
> + struct ofputil_flow_stats *);
> int
> main(int argc, char *argv[])
> {
> @@ -99,12 +113,35 @@ main(int argc, char *argv[])
> }
>
> static void
> +add_sort_criterion(enum sort_order order, const char *field)
> +{
> + struct sort_criterion *sc;
> +
> + if (n_criteria >= allocated_criteria) {
> + criteria = x2nrealloc(criteria, &allocated_criteria, sizeof
> *criteria);
> + }
> +
> + sc = &criteria[n_criteria++];
> + if (!field || !strcasecmp(field, "priority")) {
> + sc->field = NULL;
> + } else {
> + sc->field = mf_from_name(field);
> + if (!sc->field) {
> + ovs_fatal(0, "%s: unknown field name", field);
> + }
> + }
> + sc->order = order;
> +}
> +
> +static void
> parse_options(int argc, char *argv[])
> {
> enum {
> OPT_STRICT = UCHAR_MAX + 1,
> OPT_READD,
> OPT_TIMESTAMP,
> + OPT_SORT,
> + OPT_RSORT,
> DAEMON_OPTION_ENUMS,
> VLOG_OPTION_ENUMS
> };
> @@ -116,6 +153,8 @@ parse_options(int argc, char *argv[])
> {"packet-in-format", required_argument, NULL, 'P'},
> {"more", no_argument, NULL, 'm'},
> {"timestamp", no_argument, NULL, OPT_TIMESTAMP},
> + {"sort", optional_argument, NULL, OPT_SORT},
> + {"rsort", optional_argument, NULL, OPT_RSORT},
> {"help", no_argument, NULL, 'h'},
> {"version", no_argument, NULL, 'V'},
> DAEMON_LONG_OPTIONS,
> @@ -183,6 +222,14 @@ parse_options(int argc, char *argv[])
> timestamp = true;
> break;
>
> + case OPT_SORT:
> + add_sort_criterion(SORT_ASC, optarg);
> + break;
> +
> + case OPT_RSORT:
> + add_sort_criterion(SORT_DESC, optarg);
> + break;
> +
> DAEMON_OPTION_HANDLERS
> VLOG_OPTION_HANDLERS
> STREAM_SSL_OPTION_HANDLERS
> @@ -194,6 +241,12 @@ parse_options(int argc, char *argv[])
> abort();
> }
> }
> +
> + if (n_criteria) {
> + /* Always do a final sort pass based on priority. */
> + add_sort_criterion(SORT_DESC, "priority");
> + }
> +
> free(short_options);
> }
>
> @@ -244,6 +297,8 @@ usage(void)
> " -m, --more be more verbose printing
> OpenFlow\n"
> " --timestamp (monitor, snoop) print
> timestamps\n"
> " -t, --timeout=SECS give up after SECS seconds\n"
> + " --sort[=field] sort in ascending order\n"
> + " --rsort[=field] sort in descending order\n"
> " -h, --help display this help message\n"
> " -V, --version display version information\n");
> exit(EXIT_SUCCESS);
> @@ -770,12 +825,12 @@ set_protocol_for_flow_dump(struct vconn *vconn,
> }
> }
>
> -static void
> -do_dump_flows__(int argc, char *argv[], bool aggregate)
> +static struct vconn *
> +prepare_dump_flows(int argc, char *argv[], bool aggregate,
> + struct ofpbuf **requestp)
> {
> enum ofputil_protocol usable_protocols, protocol;
> struct ofputil_flow_stats_request fsr;
> - struct ofpbuf *request;
> struct vconn *vconn;
>
> parse_ofp_flow_stats_request_str(&fsr, aggregate, argc > 2 ? argv[2] :
> "");
> @@ -783,15 +838,118 @@ do_dump_flows__(int argc, char *argv[], bool aggregate)
>
> protocol = open_vconn(argv[1], &vconn);
> protocol = set_protocol_for_flow_dump(vconn, protocol, usable_protocols);
> - request = ofputil_encode_flow_stats_request(&fsr, protocol);
> + *requestp = ofputil_encode_flow_stats_request(&fsr, protocol);
> + return vconn;
> +}
> +
> +static void
> +do_dump_flows__(int argc, char *argv[], bool aggregate)
> +{
> + struct ofpbuf *request;
> + struct vconn *vconn;
> +
> + vconn = prepare_dump_flows(argc, argv, aggregate, &request);
> dump_stats_transaction__(vconn, request);
> vconn_close(vconn);
> }
>
> +static int
> +compare_flows(const void *afs_, const void *bfs_)
> +{
> + const struct ofputil_flow_stats *afs = afs_;
> + const struct ofputil_flow_stats *bfs = bfs_;
> + const struct cls_rule *a = &afs->rule;
> + const struct cls_rule *b = &bfs->rule;
> + const struct sort_criterion *sc;
> +
> + for (sc = criteria; sc < &criteria[n_criteria]; sc++) {
> + const struct mf_field *f = sc->field;
> + int ret;
> +
> + if (!f) {
> + ret = a->priority < b->priority ? -1 : a->priority > b->priority;
> + } else {
> + bool ina, inb;
> +
> + ina = mf_are_prereqs_ok(f, &a->flow) && !mf_is_all_wild(f,
> &a->wc);
> + inb = mf_are_prereqs_ok(f, &b->flow) && !mf_is_all_wild(f,
> &b->wc);
> + if (ina != inb) {
> + /* Skip the test for sc->order, so that missing fields always
> + * sort to the end whether we're sorting in ascending or
> + * descending order. */
> + return ina ? -1 : 1;
> + } else {
> + union mf_value aval, bval;
> +
> + mf_get_value(f, &a->flow, &aval);
> + mf_get_value(f, &b->flow, &bval);
> + ret = memcmp(&aval, &bval, f->n_bytes);
> + }
> + }
> +
> + if (ret) {
> + return sc->order == SORT_ASC ? ret : -ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> static void
> do_dump_flows(int argc, char *argv[])
> {
> - return do_dump_flows__(argc, argv, false);
> + if (!n_criteria) {
> + return do_dump_flows__(argc, argv, false);
> + } else {
> + struct ofputil_flow_stats *fses;
> + size_t n_fses, allocated_fses;
> + struct ofpbuf *request;
> + struct ofpbuf *reply;
> + struct vconn *vconn;
> + ovs_be32 send_xid;
> + struct ds s;
> + size_t i;
> +
> + vconn = prepare_dump_flows(argc, argv, false, &request);
> + send_xid = ((struct ofp_header *) request->data)->xid;
> + send_openflow_buffer(vconn, request);
> +
> + fses = NULL;
> + n_fses = allocated_fses = 0;
> + reply = NULL;
> + for (;;) {
> + struct ofputil_flow_stats *fs;
> +
> + if (n_fses >= allocated_fses) {
> + fses = x2nrealloc(fses, &allocated_fses, sizeof *fses);
> + }
> +
> + fs = &fses[n_fses];
> + if (!recv_flow_stats_reply(vconn, send_xid, &reply, fs)) {
> + break;
> + }
> + fs->actions = xmemdup(fs->actions,
> + fs->n_actions * sizeof *fs->actions);
> + n_fses++;
> + }
> +
> + qsort(fses, n_fses, sizeof *fses, compare_flows);
> +
> + ds_init(&s);
> + for (i = 0; i < n_fses; i++) {
> + ds_clear(&s);
> + ofp_print_flow_stats(&s, &fses[i]);
> + puts(ds_cstr(&s));
> + }
> + ds_destroy(&s);
> +
> + for (i = 0; i < n_fses; i++) {
> + free(fses[i].actions);
> + }
> + free(fses);
> +
> + vconn_close(vconn);
> + }
> }
>
> static void
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
