Skip to content

Commit

Permalink
Revert "dpctl: Expand the flow dump type filter"
Browse files Browse the repository at this point in the history 
Commit ab15e70 ("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 ab15e70.

CC: Gavi Teitz <[email protected]>
CC: Simon Horman <[email protected]>
CC: Roi Dayan <[email protected]>
CC: Aaron Conole <[email protected]>
Signed-off-by: Justin Pettit <[email protected]>
Acked-by: Ben Pfaff <[email protected]>
  • Loading branch information
@justinpettit
justinpettit committed Jul 25, 2018
1 parent f603d7d commit 494a745
Show file tree
Hide file tree
Showing 6 changed files with 63 additions and 119 deletions.
107 changes: 25 additions & 82 deletions lib/dpctl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 *
Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -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));
}
Expand All @@ -1025,7 +968,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
dpif_close(dpif);
out_free:
free(filter);
free(types_list);
free(type);
return error;
}

Expand Down
14 changes: 4 additions & 10 deletions lib/dpctl.man
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
44 changes: 29 additions & 15 deletions lib/dpif-netlink.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 *
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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);
}

Expand Down Expand Up @@ -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;
}

Expand Down
5 changes: 2 additions & 3 deletions lib/dpif-provider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)(
Expand Down
5 changes: 2 additions & 3 deletions lib/dpif.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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().
Expand Down
7 changes: 1 addition & 6 deletions lib/dpif.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 *);
Expand Down Expand Up @@ -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(
Expand Down

0 comments on commit 494a745

Please sign in to comment.