[ovs-dev] [PATCH 2/2] Revert "dpctl: Expand the flow dump type filter"

Justin Pettit jpettit at ovn.org
Wed Jul 25 21:01:32 UTC 2018


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