On Fri, Aug 10, 2018 at 11:30:08AM +0300, Gavi Teitz wrote:
> Added new types to the flow dump filter, and allowed multiple filter
> types to be passed at once, as a comma separated list. The new types
> added are:
>  * tc - specifies flows handled by the tc dp
>  * non-offloaded - specifies flows not offloaded to the HW
>  * all - specifies flows of all types
> 
> The type list is now fully parsed by the dpctl, and a new struct was
> added to dpif which enables dpctl to define which types of dumps to
> provide, rather than passing the type string and having dpif parse it.
> 
> Signed-off-by: Gavi Teitz <g...@mellanox.com>
> Acked-by: Roi Dayan <r...@mellanox.com>
> ---
>  lib/dpctl.c         |  112 +++++++++++++++++++++++++++++++++++++++-----------
>  lib/dpctl.man       |   14 +++++--
>  lib/dpif-netdev.c   |    2 +-
>  lib/dpif-netlink.c  |   45 +++++++-------------
>  lib/dpif-provider.h |    8 ++-
>  lib/dpif.c          |    5 +-
>  lib/dpif.h          |    7 +++-
>  7 files changed, 128 insertions(+), 65 deletions(-)
> 
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index c600eeb..995db29 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -792,18 +792,85 @@ format_dpif_flow(struct ds *ds, const struct dpif_flow 
> *f, struct hmap *ports,
>      format_odp_actions(ds, f->actions, f->actions_len, ports);
>  }
>  
> -static char *supported_dump_types[] = {
> -    "offloaded",
> -    "ovs",
> +struct dump_types {
> +    bool ovs;
> +    bool tc;
> +    bool offloaded;
> +    bool non_offloaded;
>  };

I am curious to know why a structure with bool fields is used
as opposed to an integer bitmask.

>  
> +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, char *type)
> +flow_passes_type_filter(const struct dpif_flow *f,
> +                        struct dump_types *dump_types)
>  {
> -    if (!strcmp(type, "offloaded")) {
> -        return f->attrs.offloaded;
> +    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;
>      }
> -    return true;
> +    return false;
>  }
>  
>  static struct hmap *
> @@ -843,9 +910,11 @@ 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;
> @@ -858,8 +927,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) && !type) {
> -            type = xstrdup(argv[--argc] + 5);
> +        } else if (!strncmp(argv[argc - 1], "type=", 5) && !types_list) {
> +            types_list = xstrdup(argv[--argc] + 5);
>          }
>      }
>  
> @@ -892,19 +961,12 @@ dpctl_dump_flows(int argc, const char *argv[], struct 
> dpctl_params *dpctl_p)
>          }
>      }
>  
> -    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;
> -        }
> +    memset(&dump_types, 0, sizeof dump_types);
> +    error = populate_dump_types(types_list, &dump_types, dpctl_p);
> +    if (error) {
> +        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
> @@ -914,7 +976,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, (type ? type : "dpctl"));
> +    flow_dump = dpif_flow_dump_create(dpif, false, &dpif_dump_types);
>      flow_dump_thread = dpif_flow_dump_thread_create(flow_dump);
>      while (dpif_flow_dump_next(flow_dump_thread, &f, 1)) {
>          if (filter) {
> @@ -950,7 +1012,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct 
> dpctl_params *dpctl_p)
>              }
>              pmd_id = f.pmd_id;
>          }
> -        if (!type || flow_passes_type_filter(&f, type)) {
> +        if (flow_passes_type_filter(&f, &dump_types)) {
>              format_dpif_flow(&ds, &f, portno_names, dpctl_p);
>              dpctl_print(dpctl_p, "%s\n", ds_cstr(&ds));
>          }
> @@ -968,7 +1030,7 @@ out_dpifclose:
>      dpif_close(dpif);
>  out_free:
>      free(filter);
> -    free(type);
> +    free(types_list);
>      return error;
>  }
>  
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index 5d987e6..e83546b 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -118,10 +118,16 @@ 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 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.
> +If \fBtype=\fItype\fR is specified, only displays flows of the specified 
> types.
> +\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.
>  .
>  .IP "\*(DX\fBadd\-flow\fR [\fIdp\fR] \fIflow actions\fR"
>  .TP
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 4b7e231..6d52db4 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3484,7 +3484,7 @@ dpif_netdev_flow_dump_cast(struct dpif_flow_dump *dump)
>  
>  static struct dpif_flow_dump *
>  dpif_netdev_flow_dump_create(const struct dpif *dpif_, bool terse,
> -                             char *type OVS_UNUSED)
> +                             struct dpif_flow_dump_types *types OVS_UNUSED)
>  {
>      struct dpif_netdev_flow_dump *dump;
>  
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index f669b11..0114f7b 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -1463,16 +1463,6 @@ 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;
> @@ -1481,7 +1471,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 */
> -    int type;                                /* Type of dump */
> +    struct dpif_flow_dump_types types;       /* Type of dump */
>  };
>  
>  static struct dpif_netlink_flow_dump *
> @@ -1496,7 +1486,7 @@ start_netdev_dump(const struct dpif *dpif_,
>  {
>      ovs_mutex_init(&dump->netdev_lock);
>  
> -    if (!(dump->type & DUMP_NETDEV_FLOWS)) {
> +    if (!(dump->types.netdev_flows)) {
>          dump->netdev_dumps_num = 0;
>          dump->netdev_dumps = NULL;
>          return;
> @@ -1510,24 +1500,21 @@ start_netdev_dump(const struct dpif *dpif_,
>      ovs_mutex_unlock(&dump->netdev_lock);
>  }
>  
> -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;
> +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);
>      }
> -
> -    return type;
>  }
>  
>  static struct dpif_flow_dump *
>  dpif_netlink_flow_dump_create(const struct dpif *dpif_, bool terse,
> -                              char *type)
> +                              struct dpif_flow_dump_types *types)
>  {
>      const struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>      struct dpif_netlink_flow_dump *dump;
> @@ -1537,9 +1524,9 @@ dpif_netlink_flow_dump_create(const struct dpif *dpif_, 
> bool terse,
>      dump = xmalloc(sizeof *dump);
>      dpif_flow_dump_init(&dump->up, dpif_);
>  
> -    dump->type = dpif_netlink_get_dump_type(type);
> +    dpif_netlink_populate_flow_dump_types(dump, types);
>  
> -    if (dump->type & DUMP_OVS_FLOWS) {
> +    if (dump->types.ovs_flows) {
>          dpif_netlink_flow_init(&request);
>          request.cmd = OVS_FLOW_CMD_GET;
>          request.dp_ifindex = dpif->dp_ifindex;
> @@ -1566,7 +1553,7 @@ dpif_netlink_flow_dump_destroy(struct dpif_flow_dump 
> *dump_)
>      unsigned int nl_status = 0;
>      int dump_status;
>  
> -    if (dump->type & DUMP_OVS_FLOWS) {
> +    if (dump->types.ovs_flows) {
>          nl_status = nl_dump_done(&dump->nl_dump);
>      }
>  
> @@ -1802,7 +1789,7 @@ dpif_netlink_flow_dump_next(struct 
> dpif_flow_dump_thread *thread_,
>          }
>      }
>  
> -    if (!(dump->type & DUMP_OVS_FLOWS)) {
> +    if (!(dump->types.ovs_flows)) {
>          return n_flows;
>      }
>  
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 62b3598..5c8b64c 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -284,9 +284,11 @@ 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 '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, char *type);
> +     * If 'types' isn't null, dumps only the flows of the passed types. */
> +    struct dpif_flow_dump *(*flow_dump_create)(
> +        const struct dpif *dpif,
> +        bool terse,
> +        struct dpif_flow_dump_types *types);

I don't think the coding style change included in the above is desired.

>      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 c267bcf..a047e3f 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1080,9 +1080,10 @@ 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, char *type)
> +dpif_flow_dump_create(const struct dpif *dpif, bool terse,
> +                      struct dpif_flow_dump_types *types)
>  {
> -    return dpif->dpif_class->flow_dump_create(dpif, terse, type);
> +    return dpif->dpif_class->flow_dump_create(dpif, terse, types);
>  }
>  
>  /* Destroys 'dump', which must have been created with 
> dpif_flow_dump_create().
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 33d2d0b..457cddc 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -512,6 +512,11 @@ 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 *);
> @@ -573,7 +578,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,
> -                                             char *type);
> +                                             struct dpif_flow_dump_types *);
>  int dpif_flow_dump_destroy(struct dpif_flow_dump *);
>  
>  struct dpif_flow_dump_thread *dpif_flow_dump_thread_create(
> -- 
> 1.7.1
> 
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to