[ovs-dev] [PATCH 2/2] Revert "dpctl: Expand the flow dump type filter"
Gavi Teitz
gavi at mellanox.com
Thu Jul 26 14:29:51 UTC 2018
From: Justin Pettit, sent: Thursday, July 26, 2018 12:02 AM:
> Commit ab15e70eb587 ("dpctl: Expand the flow dump type filter") had a number of issues with style, build breakage, and failing unit tests.
> The patch is being reverted so that they can addressed.
I acknowledge the build breakage issue, could you elaborate regarding the style issues?
As for the failing unit tests, this commit provides the means to fix unit tests that lost their relevance due to the changes introduced in commit d63ca5329ff9 ("dpctl: Properly reflect a rule's offloaded to HW state"). Are there other unit tests that are broken due to this commit?
Thanks!
-----Original Message-----
From: Justin Pettit [mailto:jpettit at ovn.org]
Sent: Thursday, July 26, 2018 12:02 AM
To: dev at openvswitch.org
Cc: Gavi Teitz <gavi at mellanox.com>; Simon Horman <simon.horman at netronome.com>; Roi Dayan <roid at mellanox.com>; Aaron Conole <aconole at redhat.com>
Subject: [PATCH 2/2] Revert "dpctl: Expand the flow dump type filter"
Commit ab15e70eb587 ("dpctl: Expand the flow dump type filter") had a number of issues with style, build breakage, and failing unit tests.
The patch is being reverted so that they can addressed.
This reverts commit ab15e70eb5878b46f8f84da940ffc915b6d74cad.
CC: Gavi Teitz <gavi at mellanox.com>
CC: Simon Horman <simon.horman at netronome.com>
CC: Roi Dayan <roid at mellanox.com>
CC: Aaron Conole <aconole at redhat.com>
Signed-off-by: Justin Pettit <jpettit at ovn.org>
---
lib/dpctl.c | 107 +++++++++++---------------------------------
lib/dpctl.man | 14 ++----
lib/dpif-netlink.c | 44 +++++++++++-------
lib/dpif-provider.h | 5 +--
lib/dpif.c | 5 +--
lib/dpif.h | 7 +--
6 files changed, 63 insertions(+), 119 deletions(-)
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 9e504c9d1fff..4f1e443f2662 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -792,80 +792,18 @@ format_dpif_flow(struct ds *ds, const struct dpif_flow *f, struct hmap *ports,
format_odp_actions(ds, f->actions, f->actions_len, ports); }
-struct dump_types {
- bool ovs;
- bool tc;
- bool offloaded;
- bool non_offloaded;
+static char *supported_dump_types[] = {
+ "offloaded",
+ "ovs",
};
-static void
-enable_all_dump_types(struct dump_types *dump_types) -{
- dump_types->ovs = true;
- dump_types->tc = true;
- dump_types->offloaded = true;
- dump_types->non_offloaded = true;
-}
-
-static int
-populate_dump_types(char *types_list, struct dump_types *dump_types,
- struct dpctl_params *dpctl_p)
-{
- if (!types_list) {
- enable_all_dump_types(dump_types);
- return 0;
- }
-
- char *current_type;
- while (types_list && types_list[0] != '\0') {
- current_type = types_list;
- size_t type_len = strcspn(current_type, ",");
- types_list += type_len + (types_list[type_len] != '\0');
- current_type[type_len] = '\0';
-
- if (!strcmp(current_type, "ovs")) {
- dump_types->ovs = true;
- } else if (!strcmp(current_type, "tc")) {
- dump_types->tc = true;
- } else if (!strcmp(current_type, "offloaded")) {
- dump_types->offloaded = true;
- } else if (!strcmp(current_type, "non-offloaded")) {
- dump_types->non_offloaded = true;
- } else if (!strcmp(current_type, "all")) {
- enable_all_dump_types(dump_types);
- } else {
- dpctl_error(dpctl_p, EINVAL, "Failed to parse type (%s)", current_type);
- return EINVAL;
- }
- }
- return 0;
-}
-
-static void
-determine_dpif_flow_dump_types(struct dump_types *dump_types,
- struct dpif_flow_dump_types *dpif_dump_types) {
- dpif_dump_types->ovs_flows = dump_types->ovs || dump_types->non_offloaded;
- dpif_dump_types->netdev_flows = dump_types->tc || dump_types->offloaded
- || dump_types->non_offloaded;
-}
-
static bool
-flow_passes_type_filter(const struct dpif_flow *f, struct dump_types *dump_types)
+flow_passes_type_filter(const struct dpif_flow *f, char *type)
{
- if (dump_types->ovs && !strcmp(f->attrs.dp_layer, "ovs")) {
- return true;
- }
- if (dump_types->tc && !strcmp(f->attrs.dp_layer, "tc")) {
- return true;
- }
- if (dump_types->offloaded && f->attrs.offloaded) {
- return true;
- }
- if (dump_types->non_offloaded && !(f->attrs.offloaded)) {
- return true;
+ if (!strcmp(type, "offloaded")) {
+ return f->attrs.offloaded;
}
- return false;
+ return true;
}
static struct hmap *
@@ -905,11 +843,9 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
struct ds ds;
char *filter = NULL;
+ char *type = NULL;
struct flow flow_filter;
struct flow_wildcards wc_filter;
- char *types_list = NULL;
- struct dump_types dump_types;
- struct dpif_flow_dump_types dpif_dump_types;
struct dpif_flow_dump_thread *flow_dump_thread;
struct dpif_flow_dump *flow_dump;
@@ -922,8 +858,8 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
lastargc = argc;
if (!strncmp(argv[argc - 1], "filter=", 7) && !filter) {
filter = xstrdup(argv[--argc] + 7);
- } else if (!strncmp(argv[argc - 1], "type=", 5) && !types_list) {
- types_list = xstrdup(argv[--argc] + 5);
+ } else if (!strncmp(argv[argc - 1], "type=", 5) && !type) {
+ type = xstrdup(argv[--argc] + 5);
}
}
@@ -956,12 +892,19 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
}
}
- memset(&dump_types, 0, sizeof dump_types);
- error = populate_dump_types(types_list, &dump_types, dpctl_p);
- if (error) {
- goto out_free;
+ if (type) {
+ error = EINVAL;
+ for (int i = 0; i < ARRAY_SIZE(supported_dump_types); i++) {
+ if (!strcmp(supported_dump_types[i], type)) {
+ error = 0;
+ break;
+ }
+ }
+ if (error) {
+ dpctl_error(dpctl_p, error, "Failed to parse type (%s)", type);
+ goto out_free;
+ }
}
- determine_dpif_flow_dump_types(&dump_types, &dpif_dump_types);
/* Make sure that these values are different. PMD_ID_NULL means that the
* pmd is unspecified (e.g. because the datapath doesn't have different @@ -971,7 +914,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
ds_init(&ds);
memset(&f, 0, sizeof f);
- flow_dump = dpif_flow_dump_create(dpif, false, &dpif_dump_types);
+ flow_dump = dpif_flow_dump_create(dpif, false, (type ? type :
+ "dpctl"));
flow_dump_thread = dpif_flow_dump_thread_create(flow_dump);
while (dpif_flow_dump_next(flow_dump_thread, &f, 1)) {
if (filter) {
@@ -1007,7 +950,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
}
pmd_id = f.pmd_id;
}
- if (flow_passes_type_filter(&f, &dump_types)) {
+ if (!type || flow_passes_type_filter(&f, type)) {
format_dpif_flow(&ds, &f, portno_names, dpctl_p);
dpctl_print(dpctl_p, "%s\n", ds_cstr(&ds));
}
@@ -1025,7 +968,7 @@ out_dpifclose:
dpif_close(dpif);
out_free:
free(filter);
- free(types_list);
+ free(type);
return error;
}
diff --git a/lib/dpctl.man b/lib/dpctl.man index b77bc3fd8ad6..5d987e62daaa 100644
--- a/lib/dpctl.man
+++ b/lib/dpctl.man
@@ -118,16 +118,10 @@ The \fIfilter\fR is also useful to match wildcarded fields in the datapath flow. As an example, \fBfilter='tcp,tp_src=100'\fR will match the datapath flow containing '\fBtcp(src=80/0xff00,dst=8080/0xff)\fR'.
.IP
-If \fBtype=\fItype\fR is specified, only displays flows of the specified type(s).
-\fItype\fR is a comma separated list, which can contain any of the following:
-.
- \fBovs\fR - displays flows handled in the ovs dp
- \fBtc\fR - displays flows handled in the tc dp
- \fBoffloaded\fR - displays flows offloaded to the HW
- \fBnon-offloaded\fR - displays flows not offloaded to the HW
- \fBall\fR - displays all the types of flows
-.IP
-By default all the types of flows are displayed.
+If \fBtype=\fItype\fR is specified, only displays flows of a specific type.
+\fItype\fR can be \fBoffloaded\fR to display only rules offloaded to
+the HW or \fBovs\fR to display only rules from the OVS tables.
+By default all rules are displayed.
.
.IP "\*(DX\fBadd\-flow\fR [\fIdp\fR] \fIflow actions\fR"
.TP
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index e0edd9c2ffb1..62c7120e8f54 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -1462,6 +1462,16 @@ dpif_netlink_init_flow_del(struct dpif_netlink *dpif,
del->ufid, del->terse, request); }
+enum {
+ DUMP_OVS_FLOWS_BIT = 0,
+ DUMP_NETDEV_FLOWS_BIT = 1,
+};
+
+enum {
+ DUMP_OVS_FLOWS = (1 << DUMP_OVS_FLOWS_BIT),
+ DUMP_NETDEV_FLOWS = (1 << DUMP_NETDEV_FLOWS_BIT), };
+
struct dpif_netlink_flow_dump {
struct dpif_flow_dump up;
struct nl_dump nl_dump;
@@ -1470,7 +1480,7 @@ struct dpif_netlink_flow_dump {
int netdev_dumps_num; /* Number of netdev_flow_dumps */
struct ovs_mutex netdev_lock; /* Guards the following. */
int netdev_current_dump OVS_GUARDED; /* Shared current dump */
- struct dpif_flow_dump_types types; /* Type of dump */
+ int type; /* Type of dump */
};
static struct dpif_netlink_flow_dump *
@@ -1485,7 +1495,7 @@ start_netdev_dump(const struct dpif *dpif_, {
ovs_mutex_init(&dump->netdev_lock);
- if (!(dump->types.netdev_flows)) {
+ if (!(dump->type & DUMP_NETDEV_FLOWS)) {
dump->netdev_dumps_num = 0;
dump->netdev_dumps = NULL;
return;
@@ -1499,20 +1509,24 @@ start_netdev_dump(const struct dpif *dpif_,
ovs_mutex_unlock(&dump->netdev_lock);
}
-static void
-dpif_netlink_populate_flow_dump_types(struct dpif_netlink_flow_dump *dump,
- struct dpif_flow_dump_types *types) {
- if (!types) {
- dump->types.ovs_flows = true;
- dump->types.netdev_flows = true;
- } else {
- memcpy(&dump->types, types, sizeof *types);
+static int
+dpif_netlink_get_dump_type(char *str) {
+ int type = 0;
+
+ if (!str || !strcmp(str, "ovs") || !strcmp(str, "dpctl")) {
+ type |= DUMP_OVS_FLOWS;
}
+ if ((netdev_is_flow_api_enabled() && !str)
+ || (str && (!strcmp(str, "offloaded") || !strcmp(str, "dpctl")))) {
+ type |= DUMP_NETDEV_FLOWS;
+ }
+
+ return type;
}
static struct dpif_flow_dump *
dpif_netlink_flow_dump_create(const struct dpif *dpif_, bool terse,
- struct dpif_flow_dump_types *types)
+ char *type)
{
const struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
struct dpif_netlink_flow_dump *dump; @@ -1522,9 +1536,9 @@ dpif_netlink_flow_dump_create(const struct dpif *dpif_, bool terse,
dump = xmalloc(sizeof *dump);
dpif_flow_dump_init(&dump->up, dpif_);
- dpif_netlink_populate_flow_dump_types(dump, types);
+ dump->type = dpif_netlink_get_dump_type(type);
- if (dump->types.ovs_flows) {
+ if (dump->type & DUMP_OVS_FLOWS) {
dpif_netlink_flow_init(&request);
request.cmd = OVS_FLOW_CMD_GET;
request.dp_ifindex = dpif->dp_ifindex; @@ -1551,7 +1565,7 @@ dpif_netlink_flow_dump_destroy(struct dpif_flow_dump *dump_)
unsigned int nl_status = 0;
int dump_status;
- if (dump->types.ovs_flows) {
+ if (dump->type & DUMP_OVS_FLOWS) {
nl_status = nl_dump_done(&dump->nl_dump);
}
@@ -1787,7 +1801,7 @@ dpif_netlink_flow_dump_next(struct dpif_flow_dump_thread *thread_,
}
}
- if (!(dump->types.ovs_flows)) {
+ if (!(dump->type & DUMP_OVS_FLOWS)) {
return n_flows;
}
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h index 0d2075bf48a4..62b3598acfc5 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -284,10 +284,9 @@ struct dpif_class {
* If 'terse' is true, then only UID and statistics will
* be returned in the dump. Otherwise, all fields will be returned.
*
- * If 'types' isn't null, dumps only the flows of the passed types. */
+ * If 'type' isn't null, dumps only the flows of the given type. */
struct dpif_flow_dump *(*flow_dump_create)(const struct dpif *dpif,
- bool terse,
- struct dpif_flow_dump_types *types);
+ bool terse, char *type);
int (*flow_dump_destroy)(struct dpif_flow_dump *dump);
struct dpif_flow_dump_thread *(*flow_dump_thread_create)( diff --git a/lib/dpif.c b/lib/dpif.c index 21bc062269aa..d78330bef3b8 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1080,10 +1080,9 @@ dpif_flow_del(struct dpif *dpif,
* This function always successfully returns a dpif_flow_dump. Error
* reporting is deferred to dpif_flow_dump_destroy(). */ struct dpif_flow_dump * -dpif_flow_dump_create(const struct dpif *dpif, bool terse,
- struct dpif_flow_dump_types *types)
+dpif_flow_dump_create(const struct dpif *dpif, bool terse, char *type)
{
- return dpif->dpif_class->flow_dump_create(dpif, terse, types);
+ return dpif->dpif_class->flow_dump_create(dpif, terse, type);
}
/* Destroys 'dump', which must have been created with dpif_flow_dump_create().
diff --git a/lib/dpif.h b/lib/dpif.h
index 457cddc11f7e..33d2d0bec333 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -512,11 +512,6 @@ struct dpif_flow_attrs {
const char *dp_layer; /* DP layer the flow is handled in. */
};
-struct dpif_flow_dump_types {
- bool ovs_flows;
- bool netdev_flows;
-};
-
void dpif_flow_stats_extract(const struct flow *, const struct dp_packet *packet,
long long int used, struct dpif_flow_stats *); void dpif_flow_stats_format(const struct dpif_flow_stats *, struct ds *); @@ -578,7 +573,7 @@ int dpif_flow_get(struct dpif *,
* All error reporting is deferred to the call to dpif_flow_dump_destroy().
*/
struct dpif_flow_dump *dpif_flow_dump_create(const struct dpif *, bool terse,
- struct dpif_flow_dump_types *);
+ char *type);
int dpif_flow_dump_destroy(struct dpif_flow_dump *);
struct dpif_flow_dump_thread *dpif_flow_dump_thread_create(
--
2.17.1
More information about the dev
mailing list