diff mbox series

[ovs-dev] dpctl: add add-flows command to dpctl

Message ID 159982714930.933624.8041886235136498204.stgit@ebuild
State Changes Requested
Headers show
Series [ovs-dev] dpctl: add add-flows command to dpctl | expand

Commit Message

Eelco Chaudron Sept. 11, 2020, 12:25 p.m. UTC
When you would like to add, modify, or delete a lot of flows in the
datapath, for example when you want to measure performance, adding
one flow at the time won't scale. This as it takes a decent amount
of time to set up the datapath connection.

This new command is in-line with the same command available in
ovs-ofctl which allows the same thing, with the only difference that
we do not verify all lines before we start execution. This allows for
a continuous add/delete stream. For example with a command like this:

python3 -c 'while True:
  for i in range(0, 1000):
    print("add in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{}) 1".format(int(i / 256), i % 256))
  for i in range(0, 1000):
    print("delete in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{})".format(int(i / 256), i % 256))' \
|  sudo utilities/ovs-dpctl add-flows -


Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
 lib/dpctl.c           |  179 ++++++++++++++++++++++++++++++++++++++++++++-----
 lib/dpctl.man         |   11 +++
 utilities/ovs-dpctl.c |    5 +
 3 files changed, 175 insertions(+), 20 deletions(-)

Comments

Aaron Conole Sept. 22, 2020, 1:21 p.m. UTC | #1
Eelco Chaudron <echaudro@redhat.com> writes:

> When you would like to add, modify, or delete a lot of flows in the
> datapath, for example when you want to measure performance, adding
> one flow at the time won't scale. This as it takes a decent amount
> of time to set up the datapath connection.
>
> This new command is in-line with the same command available in
> ovs-ofctl which allows the same thing, with the only difference that
> we do not verify all lines before we start execution. This allows for
> a continuous add/delete stream. For example with a command like this:
>
> python3 -c 'while True:
>   for i in range(0, 1000):
>     print("add in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{}) 1".format(int(i / 256), i % 256))
>   for i in range(0, 1000):
>     print("delete in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{})".format(int(i / 256), i % 256))' \
> |  sudo utilities/ovs-dpctl add-flows -
>
>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---

It may be worth mentioning this in NEWS.

Also, 'ovs-ofctl add-flows' is subtly different - there is the
possibility to do a single transaction with 'ovs-ofctl add-flows',
but this corresponding command cannot do that.  I guess that might be
what is intended by:

  All flow mods are executed in the order specified.

in the manpage.

Acked-by: Aaron Conole <aconole@redhat.com>

>  lib/dpctl.c           |  179 ++++++++++++++++++++++++++++++++++++++++++++-----
>  lib/dpctl.man         |   11 +++
>  utilities/ovs-dpctl.c |    5 +
>  3 files changed, 175 insertions(+), 20 deletions(-)
>
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index db2b1f896..eba2bdfa2 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -52,6 +52,12 @@
>  #include "openvswitch/ofp-flow.h"
>  #include "openvswitch/ofp-port.h"
>  
> +enum {
> +    DPCTL_FLOWS_ADD = 0,
> +    DPCTL_FLOWS_DEL,
> +    DPCTL_FLOWS_MOD
> +};
> +
>  typedef int dpctl_command_handler(int argc, const char *argv[],
>                                    struct dpctl_params *);
>  struct dpctl_command {
> @@ -1102,28 +1108,22 @@ out_free:
>  }
>  
>  static int
> -dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags,
> -               struct dpctl_params *dpctl_p)
> +dpctl_put_flow_dpif(struct dpif *dpif, const char *key_s,
> +                    const char *actions_s,
> +                    enum dpif_flow_put_flags flags,
> +                    struct dpctl_params *dpctl_p)
>  {
> -    const char *key_s = argv[argc - 2];
> -    const char *actions_s = argv[argc - 1];
>      struct dpif_flow_stats stats;
>      struct dpif_port dpif_port;
>      struct dpif_port_dump port_dump;
>      struct ofpbuf actions;
>      struct ofpbuf key;
>      struct ofpbuf mask;
> -    struct dpif *dpif;
>      ovs_u128 ufid;
>      bool ufid_present;
>      struct simap port_names;
>      int n, error;
>  
> -    error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
> -    if (error) {
> -        return error;
> -    }
> -
>      ufid_present = false;
>      n = odp_ufid_from_string(key_s, &ufid);
>      if (n < 0) {
> @@ -1185,6 +1185,24 @@ out_freeactions:
>  out_freekeymask:
>      ofpbuf_uninit(&mask);
>      ofpbuf_uninit(&key);
> +    return error;
> +}
> +
> +static int
> +dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags,
> +               struct dpctl_params *dpctl_p)
> +{
> +    struct dpif *dpif;
> +    int error;
> +
> +    error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
> +    if (error) {
> +        return error;
> +    }
> +
> +    error = dpctl_put_flow_dpif(dpif, argv[argc - 2], argv[argc - 1], flags,
> +                                dpctl_p);
> +
>      dpif_close(dpif);
>      return error;
>  }
> @@ -1258,25 +1276,20 @@ out:
>  }
>  
>  static int
> -dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p)
> +dpctl_del_flow_dpif(struct dpif *dpif, const char *key_s,
> +                    struct dpctl_params *dpctl_p)
>  {
> -    const char *key_s = argv[argc - 1];
>      struct dpif_flow_stats stats;
>      struct dpif_port dpif_port;
>      struct dpif_port_dump port_dump;
>      struct ofpbuf key;
>      struct ofpbuf mask; /* To be ignored. */
> -    struct dpif *dpif;
> +
>      ovs_u128 ufid;
>      bool ufid_present;
>      struct simap port_names;
>      int n, error;
>  
> -    error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
> -    if (error) {
> -        return error;
> -    }
> -
>      ufid_present = false;
>      n = odp_ufid_from_string(key_s, &ufid);
>      if (n < 0) {
> @@ -1334,6 +1347,23 @@ out:
>      ofpbuf_uninit(&mask);
>      ofpbuf_uninit(&key);
>      simap_destroy(&port_names);
> +    return error;
> +}
> +
> +static int
> +dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p)
> +{
> +    const char *key_s = argv[argc - 1];
> +    struct dpif *dpif;
> +    int error;
> +
> +    error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
> +    if (error) {
> +        return error;
> +    }
> +
> +    error = dpctl_del_flow_dpif(dpif, key_s, dpctl_p);
> +
>      dpif_close(dpif);
>      return error;
>  }
> @@ -1356,6 +1386,118 @@ dpctl_del_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
>      return error;
>  }
>  
> +static int
> +dpctl_parse_add_flows_line(struct ds *s, char **flow, char **action)
> +{
> +    int command = DPCTL_FLOWS_ADD;
> +    const char *line = ds_cstr(s);
> +    size_t len;
> +
> +    /* First figure out the command, or fallback to FLOWS_ADD. */
> +    line += strspn(line, " \t\r\n");
> +    len = strcspn(line, ", \t\r\n");
> +
> +    if (!strncmp(line, "add", len)) {
> +         command = DPCTL_FLOWS_ADD;
> +    } else if (!strncmp(line, "delete", len)) {
> +        command = DPCTL_FLOWS_DEL;
> +    } else if (!strncmp(line, "modify", len)) {
> +        command = DPCTL_FLOWS_MOD;
> +    } else {
> +        len = 0;
> +    }
> +    line += len;
> +
> +    /* Isolate flow and action (for add/modify). */
> +    line += strspn(line, " \t\r\n");
> +    len = strcspn(line, " \t\r\n");
> +
> +    if (len == 0) {
> +        *flow = NULL;
> +        *action = NULL;
> +        return command;
> +    }
> +
> +    *flow = strndup(line, len);
> +
> +    line += len;
> +    line += strspn(line, " \t\r\n");
> +    if (strlen(line)) {
> +        *action = xstrdup(line);
> +    } else {
> +        *action = NULL;
> +    }
> +
> +    return command;
> +}
> +
> +static int
> +dpctl_add_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
> +{
> +    const char *file_name = argv[argc - 1];
> +    int line_number = 0;
> +    struct dpif *dpif;
> +    struct ds line;
> +    FILE *stream;
> +    int error;
> +
> +    error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
> +    if (error) {
> +        return error;
> +    }
> +
> +    stream = !strcmp(file_name, "-") ? stdin : fopen(file_name, "r");
> +    if (!stream) {
> +        error = errno;
> +        dpctl_error(dpctl_p, error, "Opening file \"%s\" failed", file_name);
> +        goto out_close_dpif;
> +    }
> +
> +    ds_init(&line);
> +    while (!ds_get_preprocessed_line(&line, stream, &line_number)) {
> +        /* We do not process all the lines first and then execute the actions
> +         * as we would like to take commands as a continuous stream of
> +         * commands from stdin.
> +         */
> +        char *flow = NULL;
> +        char *action = NULL;
> +        int cmd = dpctl_parse_add_flows_line(&line, &flow, &action);
> +
> +        if ((!flow && !action)
> +            || ((cmd == DPCTL_FLOWS_ADD || cmd == DPCTL_FLOWS_MOD) && !action)
> +            || (cmd == DPCTL_FLOWS_DEL && action)) {
> +            dpctl_error(dpctl_p, 0,
> +                        "Failed parsing line number %u, skipped!",
> +                        line_number);
> +        } else {
> +            switch (cmd) {
> +            case DPCTL_FLOWS_ADD:
> +                dpctl_put_flow_dpif(dpif, flow, action,
> +                                    DPIF_FP_CREATE, dpctl_p);
> +                break;
> +            case DPCTL_FLOWS_MOD:
> +                dpctl_put_flow_dpif(dpif, flow, action,
> +                                    DPIF_FP_MODIFY, dpctl_p);
> +                break;
> +            case DPCTL_FLOWS_DEL:
> +                dpctl_del_flow_dpif(dpif, flow, dpctl_p);
> +                break;
> +            }
> +        }
> +
> +        free(flow);
> +        free(action);
> +    }
> +
> +    ds_destroy(&line);
> +    if (stream != stdin) {
> +        fclose(stream);
> +    }
> +out_close_dpif:
> +    dpif_close(dpif);
> +    return 0;
> +}
> +
>  static int
>  dpctl_help(int argc OVS_UNUSED, const char *argv[] OVS_UNUSED,
>             struct dpctl_params *dpctl_p)
> @@ -2506,6 +2648,7 @@ static const struct dpctl_command all_commands[] = {
>      { "dump-flows", "[dp] [filter=..] [type=..]",
>        0, 3, dpctl_dump_flows, DP_RO },
>      { "add-flow", "[dp] flow actions", 2, 3, dpctl_add_flow, DP_RW },
> +    { "add-flows", "[dp] file", 1, 2, dpctl_add_flows, DP_RW },
>      { "mod-flow", "[dp] flow actions", 2, 3, dpctl_mod_flow, DP_RW },
>      { "get-flow", "[dp] ufid", 1, 2, dpctl_get_flow, DP_RO },
>      { "del-flow", "[dp] flow", 1, 2, dpctl_del_flow, DP_RW },
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index 727d1f7be..e384e00da 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -194,6 +194,17 @@ ovs-dpctl add-flow myDP \\
>  .
>  .RE
>  .TP
> +.DO "\*(DX\fBadd\-flows\fR [\fIdp\fR] \fIfile\fR"
> +Reads flow entries from \fIfile\fR (or \fBstdin\fR if \fIfile\fR is
> +\fB\-\fR) and adds each entry to the datapath.
> +.
> +Each flow specification (e.g., each line in \fIfile\fR) may start with
> +\fBadd\fR, \fBmodify\fR, or \fBdelete\fR keyword to specify whether a
> +flow is to be added, modified, or deleted. A flow specification without
> +one of these keywords is treated as a flow add.  All flow mods are
> +executed in the order specified.
> +.
> +.TP
>  .DO "[\fB\-s\fR | \fB\-\-statistics\fR]" "\*(DX\fBdel\-flow\fR" "[\fIdp\fR] \fIflow\fR"
>  Deletes the flow from \fIdp\fR's flow table that matches \fIflow\fR.
>  If \fB\-s\fR or \fB\-\-statistics\fR is specified, then
> diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
> index 7d99607f4..3ff3f714c 100644
> --- a/utilities/ovs-dpctl.c
> +++ b/utilities/ovs-dpctl.c
> @@ -191,6 +191,7 @@ usage(void *userdata OVS_UNUSED)
>             "  show DP...               show basic info on each DP\n"
>             "  dump-flows [DP]          display flows in DP\n"
>             "  add-flow [DP] FLOW ACTIONS add FLOW with ACTIONS to DP\n"
> +           "  add-flows [DP] FILE        add flows from FILE\n"
>             "  mod-flow [DP] FLOW ACTIONS change FLOW actions to ACTIONS in DP\n"
>             "  get-flow [DP] ufid:UFID    fetch flow corresponding to UFID\n"
>             "  del-flow [DP] FLOW         delete FLOW from DP\n"
> @@ -205,8 +206,8 @@ usage(void *userdata OVS_UNUSED)
>             "Each IFACE on add-dp, add-if, and set-if may be followed by\n"
>             "comma-separated options.  See ovs-dpctl(8) for syntax, or the\n"
>             "Interface table in ovs-vswitchd.conf.db(5) for an options list.\n"
> -           "For COMMAND dump-flows, add-flow, mod-flow, del-flow and\n"
> -           "del-flows, DP is optional if there is only one datapath.\n",
> +           "For COMMAND dump-flows, add-flow, add-flows, mod-flow, del-flow\n"
> +           "and del-flows, DP is optional if there is only one datapath.\n",
>             program_name, program_name);
>      vlog_usage();
>      printf("\nOptions for show and mod-flow:\n"
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Eelco Chaudron Sept. 28, 2020, 9:51 a.m. UTC | #2
On 22 Sep 2020, at 15:21, Aaron Conole wrote:

> Eelco Chaudron <echaudro@redhat.com> writes:
>
>> When you would like to add, modify, or delete a lot of flows in the
>> datapath, for example when you want to measure performance, adding
>> one flow at the time won't scale. This as it takes a decent amount
>> of time to set up the datapath connection.
>>
>> This new command is in-line with the same command available in
>> ovs-ofctl which allows the same thing, with the only difference that
>> we do not verify all lines before we start execution. This allows for
>> a continuous add/delete stream. For example with a command like this:
>>
>> python3 -c 'while True:
>>   for i in range(0, 1000):
>>     print("add in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{}) 
>> 1".format(int(i / 256), i % 256))
>>   for i in range(0, 1000):
>>     print("delete 
>> in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{})".format(int(i 
>> / 256), i % 256))' \
>> |  sudo utilities/ovs-dpctl add-flows -
>>
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
>
> It may be worth mentioning this in NEWS.

Good suggestion, will send out a v2 later.

>
> Also, 'ovs-ofctl add-flows' is subtly different - there is the
> possibility to do a single transaction with 'ovs-ofctl add-flows',
> but this corresponding command cannot do that.  I guess that might be
> what is intended by:
>
>   All flow mods are executed in the order specified.
>
> in the manpage.

Not sure what you mean with single transaction? The following does work:

   echo "in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.0.2) 0" | 
ovs-dpctl add-flows test -


> Acked-by: Aaron Conole <aconole@redhat.com>
>
>>  lib/dpctl.c           |  179 
>> ++++++++++++++++++++++++++++++++++++++++++++-----
>>  lib/dpctl.man         |   11 +++
>>  utilities/ovs-dpctl.c |    5 +
>>  3 files changed, 175 insertions(+), 20 deletions(-)
>>
>> diff --git a/lib/dpctl.c b/lib/dpctl.c
>> index db2b1f896..eba2bdfa2 100644
>> --- a/lib/dpctl.c
>> +++ b/lib/dpctl.c
>> @@ -52,6 +52,12 @@
>>  #include "openvswitch/ofp-flow.h"
>>  #include "openvswitch/ofp-port.h"
>>
>> +enum {
>> +    DPCTL_FLOWS_ADD = 0,
>> +    DPCTL_FLOWS_DEL,
>> +    DPCTL_FLOWS_MOD
>> +};
>> +
>>  typedef int dpctl_command_handler(int argc, const char *argv[],
>>                                    struct dpctl_params *);
>>  struct dpctl_command {
>> @@ -1102,28 +1108,22 @@ out_free:
>>  }
>>
>>  static int
>> -dpctl_put_flow(int argc, const char *argv[], enum 
>> dpif_flow_put_flags flags,
>> -               struct dpctl_params *dpctl_p)
>> +dpctl_put_flow_dpif(struct dpif *dpif, const char *key_s,
>> +                    const char *actions_s,
>> +                    enum dpif_flow_put_flags flags,
>> +                    struct dpctl_params *dpctl_p)
>>  {
>> -    const char *key_s = argv[argc - 2];
>> -    const char *actions_s = argv[argc - 1];
>>      struct dpif_flow_stats stats;
>>      struct dpif_port dpif_port;
>>      struct dpif_port_dump port_dump;
>>      struct ofpbuf actions;
>>      struct ofpbuf key;
>>      struct ofpbuf mask;
>> -    struct dpif *dpif;
>>      ovs_u128 ufid;
>>      bool ufid_present;
>>      struct simap port_names;
>>      int n, error;
>>
>> -    error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
>> -    if (error) {
>> -        return error;
>> -    }
>> -
>>      ufid_present = false;
>>      n = odp_ufid_from_string(key_s, &ufid);
>>      if (n < 0) {
>> @@ -1185,6 +1185,24 @@ out_freeactions:
>>  out_freekeymask:
>>      ofpbuf_uninit(&mask);
>>      ofpbuf_uninit(&key);
>> +    return error;
>> +}
>> +
>> +static int
>> +dpctl_put_flow(int argc, const char *argv[], enum 
>> dpif_flow_put_flags flags,
>> +               struct dpctl_params *dpctl_p)
>> +{
>> +    struct dpif *dpif;
>> +    int error;
>> +
>> +    error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
>> +    if (error) {
>> +        return error;
>> +    }
>> +
>> +    error = dpctl_put_flow_dpif(dpif, argv[argc - 2], argv[argc - 
>> 1], flags,
>> +                                dpctl_p);
>> +
>>      dpif_close(dpif);
>>      return error;
>>  }
>> @@ -1258,25 +1276,20 @@ out:
>>  }
>>
>>  static int
>> -dpctl_del_flow(int argc, const char *argv[], struct dpctl_params 
>> *dpctl_p)
>> +dpctl_del_flow_dpif(struct dpif *dpif, const char *key_s,
>> +                    struct dpctl_params *dpctl_p)
>>  {
>> -    const char *key_s = argv[argc - 1];
>>      struct dpif_flow_stats stats;
>>      struct dpif_port dpif_port;
>>      struct dpif_port_dump port_dump;
>>      struct ofpbuf key;
>>      struct ofpbuf mask; /* To be ignored. */
>> -    struct dpif *dpif;
>> +
>>      ovs_u128 ufid;
>>      bool ufid_present;
>>      struct simap port_names;
>>      int n, error;
>>
>> -    error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
>> -    if (error) {
>> -        return error;
>> -    }
>> -
>>      ufid_present = false;
>>      n = odp_ufid_from_string(key_s, &ufid);
>>      if (n < 0) {
>> @@ -1334,6 +1347,23 @@ out:
>>      ofpbuf_uninit(&mask);
>>      ofpbuf_uninit(&key);
>>      simap_destroy(&port_names);
>> +    return error;
>> +}
>> +
>> +static int
>> +dpctl_del_flow(int argc, const char *argv[], struct dpctl_params 
>> *dpctl_p)
>> +{
>> +    const char *key_s = argv[argc - 1];
>> +    struct dpif *dpif;
>> +    int error;
>> +
>> +    error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
>> +    if (error) {
>> +        return error;
>> +    }
>> +
>> +    error = dpctl_del_flow_dpif(dpif, key_s, dpctl_p);
>> +
>>      dpif_close(dpif);
>>      return error;
>>  }
>> @@ -1356,6 +1386,118 @@ dpctl_del_flows(int argc, const char *argv[], 
>> struct dpctl_params *dpctl_p)
>>      return error;
>>  }
>>
>> +static int
>> +dpctl_parse_add_flows_line(struct ds *s, char **flow, char **action)
>> +{
>> +    int command = DPCTL_FLOWS_ADD;
>> +    const char *line = ds_cstr(s);
>> +    size_t len;
>> +
>> +    /* First figure out the command, or fallback to FLOWS_ADD. */
>> +    line += strspn(line, " \t\r\n");
>> +    len = strcspn(line, ", \t\r\n");
>> +
>> +    if (!strncmp(line, "add", len)) {
>> +         command = DPCTL_FLOWS_ADD;
>> +    } else if (!strncmp(line, "delete", len)) {
>> +        command = DPCTL_FLOWS_DEL;
>> +    } else if (!strncmp(line, "modify", len)) {
>> +        command = DPCTL_FLOWS_MOD;
>> +    } else {
>> +        len = 0;
>> +    }
>> +    line += len;
>> +
>> +    /* Isolate flow and action (for add/modify). */
>> +    line += strspn(line, " \t\r\n");
>> +    len = strcspn(line, " \t\r\n");
>> +
>> +    if (len == 0) {
>> +        *flow = NULL;
>> +        *action = NULL;
>> +        return command;
>> +    }
>> +
>> +    *flow = strndup(line, len);
>> +
>> +    line += len;
>> +    line += strspn(line, " \t\r\n");
>> +    if (strlen(line)) {
>> +        *action = xstrdup(line);
>> +    } else {
>> +        *action = NULL;
>> +    }
>> +
>> +    return command;
>> +}
>> +
>> +static int
>> +dpctl_add_flows(int argc, const char *argv[], struct dpctl_params 
>> *dpctl_p)
>> +{
>> +    const char *file_name = argv[argc - 1];
>> +    int line_number = 0;
>> +    struct dpif *dpif;
>> +    struct ds line;
>> +    FILE *stream;
>> +    int error;
>> +
>> +    error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
>> +    if (error) {
>> +        return error;
>> +    }
>> +
>> +    stream = !strcmp(file_name, "-") ? stdin : fopen(file_name, 
>> "r");
>> +    if (!stream) {
>> +        error = errno;
>> +        dpctl_error(dpctl_p, error, "Opening file \"%s\" failed", 
>> file_name);
>> +        goto out_close_dpif;
>> +    }
>> +
>> +    ds_init(&line);
>> +    while (!ds_get_preprocessed_line(&line, stream, &line_number)) {
>> +        /* We do not process all the lines first and then execute 
>> the actions
>> +         * as we would like to take commands as a continuous stream 
>> of
>> +         * commands from stdin.
>> +         */
>> +        char *flow = NULL;
>> +        char *action = NULL;
>> +        int cmd = dpctl_parse_add_flows_line(&line, &flow, &action);
>> +
>> +        if ((!flow && !action)
>> +            || ((cmd == DPCTL_FLOWS_ADD || cmd == DPCTL_FLOWS_MOD) 
>> && !action)
>> +            || (cmd == DPCTL_FLOWS_DEL && action)) {
>> +            dpctl_error(dpctl_p, 0,
>> +                        "Failed parsing line number %u, skipped!",
>> +                        line_number);
>> +        } else {
>> +            switch (cmd) {
>> +            case DPCTL_FLOWS_ADD:
>> +                dpctl_put_flow_dpif(dpif, flow, action,
>> +                                    DPIF_FP_CREATE, dpctl_p);
>> +                break;
>> +            case DPCTL_FLOWS_MOD:
>> +                dpctl_put_flow_dpif(dpif, flow, action,
>> +                                    DPIF_FP_MODIFY, dpctl_p);
>> +                break;
>> +            case DPCTL_FLOWS_DEL:
>> +                dpctl_del_flow_dpif(dpif, flow, dpctl_p);
>> +                break;
>> +            }
>> +        }
>> +
>> +        free(flow);
>> +        free(action);
>> +    }
>> +
>> +    ds_destroy(&line);
>> +    if (stream != stdin) {
>> +        fclose(stream);
>> +    }
>> +out_close_dpif:
>> +    dpif_close(dpif);
>> +    return 0;
>> +}
>> +
>>  static int
>>  dpctl_help(int argc OVS_UNUSED, const char *argv[] OVS_UNUSED,
>>             struct dpctl_params *dpctl_p)
>> @@ -2506,6 +2648,7 @@ static const struct dpctl_command 
>> all_commands[] = {
>>      { "dump-flows", "[dp] [filter=..] [type=..]",
>>        0, 3, dpctl_dump_flows, DP_RO },
>>      { "add-flow", "[dp] flow actions", 2, 3, dpctl_add_flow, DP_RW 
>> },
>> +    { "add-flows", "[dp] file", 1, 2, dpctl_add_flows, DP_RW },
>>      { "mod-flow", "[dp] flow actions", 2, 3, dpctl_mod_flow, DP_RW 
>> },
>>      { "get-flow", "[dp] ufid", 1, 2, dpctl_get_flow, DP_RO },
>>      { "del-flow", "[dp] flow", 1, 2, dpctl_del_flow, DP_RW },
>> diff --git a/lib/dpctl.man b/lib/dpctl.man
>> index 727d1f7be..e384e00da 100644
>> --- a/lib/dpctl.man
>> +++ b/lib/dpctl.man
>> @@ -194,6 +194,17 @@ ovs-dpctl add-flow myDP \\
>>  .
>>  .RE
>>  .TP
>> +.DO "\*(DX\fBadd\-flows\fR [\fIdp\fR] \fIfile\fR"
>> +Reads flow entries from \fIfile\fR (or \fBstdin\fR if \fIfile\fR is
>> +\fB\-\fR) and adds each entry to the datapath.
>> +.
>> +Each flow specification (e.g., each line in \fIfile\fR) may start 
>> with
>> +\fBadd\fR, \fBmodify\fR, or \fBdelete\fR keyword to specify whether 
>> a
>> +flow is to be added, modified, or deleted. A flow specification 
>> without
>> +one of these keywords is treated as a flow add.  All flow mods are
>> +executed in the order specified.
>> +.
>> +.TP
>>  .DO "[\fB\-s\fR | \fB\-\-statistics\fR]" "\*(DX\fBdel\-flow\fR" 
>> "[\fIdp\fR] \fIflow\fR"
>>  Deletes the flow from \fIdp\fR's flow table that matches \fIflow\fR.
>>  If \fB\-s\fR or \fB\-\-statistics\fR is specified, then
>> diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
>> index 7d99607f4..3ff3f714c 100644
>> --- a/utilities/ovs-dpctl.c
>> +++ b/utilities/ovs-dpctl.c
>> @@ -191,6 +191,7 @@ usage(void *userdata OVS_UNUSED)
>>             "  show DP...               show basic info on each DP\n"
>>             "  dump-flows [DP]          display flows in DP\n"
>>             "  add-flow [DP] FLOW ACTIONS add FLOW with ACTIONS to 
>> DP\n"
>> +           "  add-flows [DP] FILE        add flows from FILE\n"
>>             "  mod-flow [DP] FLOW ACTIONS change FLOW actions to 
>> ACTIONS in DP\n"
>>             "  get-flow [DP] ufid:UFID    fetch flow corresponding to 
>> UFID\n"
>>             "  del-flow [DP] FLOW         delete FLOW from DP\n"
>> @@ -205,8 +206,8 @@ usage(void *userdata OVS_UNUSED)
>>             "Each IFACE on add-dp, add-if, and set-if may be followed 
>> by\n"
>>             "comma-separated options.  See ovs-dpctl(8) for syntax, 
>> or the\n"
>>             "Interface table in ovs-vswitchd.conf.db(5) for an 
>> options list.\n"
>> -           "For COMMAND dump-flows, add-flow, mod-flow, del-flow 
>> and\n"
>> -           "del-flows, DP is optional if there is only one 
>> datapath.\n",
>> +           "For COMMAND dump-flows, add-flow, add-flows, mod-flow, 
>> del-flow\n"
>> +           "and del-flows, DP is optional if there is only one 
>> datapath.\n",
>>             program_name, program_name);
>>      vlog_usage();
>>      printf("\nOptions for show and mod-flow:\n"
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Aaron Conole Sept. 30, 2020, 1:06 p.m. UTC | #3
"Eelco Chaudron" <echaudro@redhat.com> writes:

> On 22 Sep 2020, at 15:21, Aaron Conole wrote:
>
>> Eelco Chaudron <echaudro@redhat.com> writes:
>>
>>> When you would like to add, modify, or delete a lot of flows in the
>>> datapath, for example when you want to measure performance, adding
>>> one flow at the time won't scale. This as it takes a decent amount
>>> of time to set up the datapath connection.
>>>
>>> This new command is in-line with the same command available in
>>> ovs-ofctl which allows the same thing, with the only difference that
>>> we do not verify all lines before we start execution. This allows for
>>> a continuous add/delete stream. For example with a command like this:
>>>
>>> python3 -c 'while True:
>>>   for i in range(0, 1000):
>>>     print("add
>>> in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{})
>>> 1".format(int(i / 256), i % 256))
>>>   for i in range(0, 1000):
>>>     print("delete
>>> in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{})".format(int(i
>>> / 256), i % 256))' \
>>> |  sudo utilities/ovs-dpctl add-flows -
>>>
>>>
>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>>> ---
>>
>> It may be worth mentioning this in NEWS.
>
> Good suggestion, will send out a v2 later.
>
>>
>> Also, 'ovs-ofctl add-flows' is subtly different - there is the
>> possibility to do a single transaction with 'ovs-ofctl add-flows',
>> but this corresponding command cannot do that.  I guess that might be
>> what is intended by:
>>
>>   All flow mods are executed in the order specified.
>>
>> in the manpage.
>
> Not sure what you mean with single transaction? The following does work:
>
>   echo "in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.0.2) 0" |
> ovs-dpctl add-flows test -

IIRC, all the flow rules can be added as part of a single openflow
transaction with ovs-ofctl.  So multiple flow rules will be committed at
exactly the same time (--bundle, etc).

This command will only ever do 1 at a time.  That's okay, but maybe it's
worth pointing out this difference.

>> Acked-by: Aaron Conole <aconole@redhat.com>
>>
>>>  lib/dpctl.c           |  179
>>> ++++++++++++++++++++++++++++++++++++++++++++-----
>>>  lib/dpctl.man         |   11 +++
>>>  utilities/ovs-dpctl.c |    5 +
>>>  3 files changed, 175 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/lib/dpctl.c b/lib/dpctl.c
>>> index db2b1f896..eba2bdfa2 100644
>>> --- a/lib/dpctl.c
>>> +++ b/lib/dpctl.c
>>> @@ -52,6 +52,12 @@
>>>  #include "openvswitch/ofp-flow.h"
>>>  #include "openvswitch/ofp-port.h"
>>>
>>> +enum {
>>> +    DPCTL_FLOWS_ADD = 0,
>>> +    DPCTL_FLOWS_DEL,
>>> +    DPCTL_FLOWS_MOD
>>> +};
>>> +
>>>  typedef int dpctl_command_handler(int argc, const char *argv[],
>>>                                    struct dpctl_params *);
>>>  struct dpctl_command {
>>> @@ -1102,28 +1108,22 @@ out_free:
>>>  }
>>>
>>>  static int
>>> -dpctl_put_flow(int argc, const char *argv[], enum
>>> dpif_flow_put_flags flags,
>>> -               struct dpctl_params *dpctl_p)
>>> +dpctl_put_flow_dpif(struct dpif *dpif, const char *key_s,
>>> +                    const char *actions_s,
>>> +                    enum dpif_flow_put_flags flags,
>>> +                    struct dpctl_params *dpctl_p)
>>>  {
>>> -    const char *key_s = argv[argc - 2];
>>> -    const char *actions_s = argv[argc - 1];
>>>      struct dpif_flow_stats stats;
>>>      struct dpif_port dpif_port;
>>>      struct dpif_port_dump port_dump;
>>>      struct ofpbuf actions;
>>>      struct ofpbuf key;
>>>      struct ofpbuf mask;
>>> -    struct dpif *dpif;
>>>      ovs_u128 ufid;
>>>      bool ufid_present;
>>>      struct simap port_names;
>>>      int n, error;
>>>
>>> -    error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
>>> -    if (error) {
>>> -        return error;
>>> -    }
>>> -
>>>      ufid_present = false;
>>>      n = odp_ufid_from_string(key_s, &ufid);
>>>      if (n < 0) {
>>> @@ -1185,6 +1185,24 @@ out_freeactions:
>>>  out_freekeymask:
>>>      ofpbuf_uninit(&mask);
>>>      ofpbuf_uninit(&key);
>>> +    return error;
>>> +}
>>> +
>>> +static int
>>> +dpctl_put_flow(int argc, const char *argv[], enum
>>> dpif_flow_put_flags flags,
>>> +               struct dpctl_params *dpctl_p)
>>> +{
>>> +    struct dpif *dpif;
>>> +    int error;
>>> +
>>> +    error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
>>> +    if (error) {
>>> +        return error;
>>> +    }
>>> +
>>> +    error = dpctl_put_flow_dpif(dpif, argv[argc - 2], argv[argc - 
>>> 1], flags,
>>> +                                dpctl_p);
>>> +
>>>      dpif_close(dpif);
>>>      return error;
>>>  }
>>> @@ -1258,25 +1276,20 @@ out:
>>>  }
>>>
>>>  static int
>>> -dpctl_del_flow(int argc, const char *argv[], struct dpctl_params
>>> *dpctl_p)
>>> +dpctl_del_flow_dpif(struct dpif *dpif, const char *key_s,
>>> +                    struct dpctl_params *dpctl_p)
>>>  {
>>> -    const char *key_s = argv[argc - 1];
>>>      struct dpif_flow_stats stats;
>>>      struct dpif_port dpif_port;
>>>      struct dpif_port_dump port_dump;
>>>      struct ofpbuf key;
>>>      struct ofpbuf mask; /* To be ignored. */
>>> -    struct dpif *dpif;
>>> +
>>>      ovs_u128 ufid;
>>>      bool ufid_present;
>>>      struct simap port_names;
>>>      int n, error;
>>>
>>> -    error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
>>> -    if (error) {
>>> -        return error;
>>> -    }
>>> -
>>>      ufid_present = false;
>>>      n = odp_ufid_from_string(key_s, &ufid);
>>>      if (n < 0) {
>>> @@ -1334,6 +1347,23 @@ out:
>>>      ofpbuf_uninit(&mask);
>>>      ofpbuf_uninit(&key);
>>>      simap_destroy(&port_names);
>>> +    return error;
>>> +}
>>> +
>>> +static int
>>> +dpctl_del_flow(int argc, const char *argv[], struct dpctl_params
>>> *dpctl_p)
>>> +{
>>> +    const char *key_s = argv[argc - 1];
>>> +    struct dpif *dpif;
>>> +    int error;
>>> +
>>> +    error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
>>> +    if (error) {
>>> +        return error;
>>> +    }
>>> +
>>> +    error = dpctl_del_flow_dpif(dpif, key_s, dpctl_p);
>>> +
>>>      dpif_close(dpif);
>>>      return error;
>>>  }
>>> @@ -1356,6 +1386,118 @@ dpctl_del_flows(int argc, const char
>>> *argv[], struct dpctl_params *dpctl_p)
>>>      return error;
>>>  }
>>>
>>> +static int
>>> +dpctl_parse_add_flows_line(struct ds *s, char **flow, char **action)
>>> +{
>>> +    int command = DPCTL_FLOWS_ADD;
>>> +    const char *line = ds_cstr(s);
>>> +    size_t len;
>>> +
>>> +    /* First figure out the command, or fallback to FLOWS_ADD. */
>>> +    line += strspn(line, " \t\r\n");
>>> +    len = strcspn(line, ", \t\r\n");
>>> +
>>> +    if (!strncmp(line, "add", len)) {
>>> +         command = DPCTL_FLOWS_ADD;
>>> +    } else if (!strncmp(line, "delete", len)) {
>>> +        command = DPCTL_FLOWS_DEL;
>>> +    } else if (!strncmp(line, "modify", len)) {
>>> +        command = DPCTL_FLOWS_MOD;
>>> +    } else {
>>> +        len = 0;
>>> +    }
>>> +    line += len;
>>> +
>>> +    /* Isolate flow and action (for add/modify). */
>>> +    line += strspn(line, " \t\r\n");
>>> +    len = strcspn(line, " \t\r\n");
>>> +
>>> +    if (len == 0) {
>>> +        *flow = NULL;
>>> +        *action = NULL;
>>> +        return command;
>>> +    }
>>> +
>>> +    *flow = strndup(line, len);
>>> +
>>> +    line += len;
>>> +    line += strspn(line, " \t\r\n");
>>> +    if (strlen(line)) {
>>> +        *action = xstrdup(line);
>>> +    } else {
>>> +        *action = NULL;
>>> +    }
>>> +
>>> +    return command;
>>> +}
>>> +
>>> +static int
>>> +dpctl_add_flows(int argc, const char *argv[], struct dpctl_params
>>> *dpctl_p)
>>> +{
>>> +    const char *file_name = argv[argc - 1];
>>> +    int line_number = 0;
>>> +    struct dpif *dpif;
>>> +    struct ds line;
>>> +    FILE *stream;
>>> +    int error;
>>> +
>>> +    error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
>>> +    if (error) {
>>> +        return error;
>>> +    }
>>> +
>>> +    stream = !strcmp(file_name, "-") ? stdin : fopen(file_name,
>>> "r");
>>> +    if (!stream) {
>>> +        error = errno;
>>> +        dpctl_error(dpctl_p, error, "Opening file \"%s\" failed",
>>> file_name);
>>> +        goto out_close_dpif;
>>> +    }
>>> +
>>> +    ds_init(&line);
>>> +    while (!ds_get_preprocessed_line(&line, stream, &line_number)) {
>>> +        /* We do not process all the lines first and then execute
>>> the actions
>>> +         * as we would like to take commands as a continuous
>>> stream of
>>> +         * commands from stdin.
>>> +         */
>>> +        char *flow = NULL;
>>> +        char *action = NULL;
>>> +        int cmd = dpctl_parse_add_flows_line(&line, &flow, &action);
>>> +
>>> +        if ((!flow && !action)
>>> +            || ((cmd == DPCTL_FLOWS_ADD || cmd == DPCTL_FLOWS_MOD)
>>> && !action)
>>> +            || (cmd == DPCTL_FLOWS_DEL && action)) {
>>> +            dpctl_error(dpctl_p, 0,
>>> +                        "Failed parsing line number %u, skipped!",
>>> +                        line_number);
>>> +        } else {
>>> +            switch (cmd) {
>>> +            case DPCTL_FLOWS_ADD:
>>> +                dpctl_put_flow_dpif(dpif, flow, action,
>>> +                                    DPIF_FP_CREATE, dpctl_p);
>>> +                break;
>>> +            case DPCTL_FLOWS_MOD:
>>> +                dpctl_put_flow_dpif(dpif, flow, action,
>>> +                                    DPIF_FP_MODIFY, dpctl_p);
>>> +                break;
>>> +            case DPCTL_FLOWS_DEL:
>>> +                dpctl_del_flow_dpif(dpif, flow, dpctl_p);
>>> +                break;
>>> +            }
>>> +        }
>>> +
>>> +        free(flow);
>>> +        free(action);
>>> +    }
>>> +
>>> +    ds_destroy(&line);
>>> +    if (stream != stdin) {
>>> +        fclose(stream);
>>> +    }
>>> +out_close_dpif:
>>> +    dpif_close(dpif);
>>> +    return 0;
>>> +}
>>> +
>>>  static int
>>>  dpctl_help(int argc OVS_UNUSED, const char *argv[] OVS_UNUSED,
>>>             struct dpctl_params *dpctl_p)
>>> @@ -2506,6 +2648,7 @@ static const struct dpctl_command
>>> all_commands[] = {
>>>      { "dump-flows", "[dp] [filter=..] [type=..]",
>>>        0, 3, dpctl_dump_flows, DP_RO },
>>>      { "add-flow", "[dp] flow actions", 2, 3, dpctl_add_flow, DP_RW
>>> },
>>> +    { "add-flows", "[dp] file", 1, 2, dpctl_add_flows, DP_RW },
>>>      { "mod-flow", "[dp] flow actions", 2, 3, dpctl_mod_flow, DP_RW
>>> },
>>>      { "get-flow", "[dp] ufid", 1, 2, dpctl_get_flow, DP_RO },
>>>      { "del-flow", "[dp] flow", 1, 2, dpctl_del_flow, DP_RW },
>>> diff --git a/lib/dpctl.man b/lib/dpctl.man
>>> index 727d1f7be..e384e00da 100644
>>> --- a/lib/dpctl.man
>>> +++ b/lib/dpctl.man
>>> @@ -194,6 +194,17 @@ ovs-dpctl add-flow myDP \\
>>>  .
>>>  .RE
>>>  .TP
>>> +.DO "\*(DX\fBadd\-flows\fR [\fIdp\fR] \fIfile\fR"
>>> +Reads flow entries from \fIfile\fR (or \fBstdin\fR if \fIfile\fR is
>>> +\fB\-\fR) and adds each entry to the datapath.
>>> +.
>>> +Each flow specification (e.g., each line in \fIfile\fR) may start
>>> with
>>> +\fBadd\fR, \fBmodify\fR, or \fBdelete\fR keyword to specify
>>> whether a
>>> +flow is to be added, modified, or deleted. A flow specification
>>> without
>>> +one of these keywords is treated as a flow add.  All flow mods are
>>> +executed in the order specified.
>>> +.
>>> +.TP
>>>  .DO "[\fB\-s\fR | \fB\-\-statistics\fR]" "\*(DX\fBdel\-flow\fR"
>>> "[\fIdp\fR] \fIflow\fR"
>>>  Deletes the flow from \fIdp\fR's flow table that matches \fIflow\fR.
>>>  If \fB\-s\fR or \fB\-\-statistics\fR is specified, then
>>> diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
>>> index 7d99607f4..3ff3f714c 100644
>>> --- a/utilities/ovs-dpctl.c
>>> +++ b/utilities/ovs-dpctl.c
>>> @@ -191,6 +191,7 @@ usage(void *userdata OVS_UNUSED)
>>>             "  show DP...               show basic info on each DP\n"
>>>             "  dump-flows [DP]          display flows in DP\n"
>>>             "  add-flow [DP] FLOW ACTIONS add FLOW with ACTIONS to
>>> DP\n"
>>> +           "  add-flows [DP] FILE        add flows from FILE\n"
>>>             "  mod-flow [DP] FLOW ACTIONS change FLOW actions to
>>> ACTIONS in DP\n"
>>>             "  get-flow [DP] ufid:UFID    fetch flow corresponding
>>> to UFID\n"
>>>             "  del-flow [DP] FLOW         delete FLOW from DP\n"
>>> @@ -205,8 +206,8 @@ usage(void *userdata OVS_UNUSED)
>>>             "Each IFACE on add-dp, add-if, and set-if may be
>>> followed by\n"
>>>             "comma-separated options.  See ovs-dpctl(8) for syntax,
>>> or the\n"
>>>             "Interface table in ovs-vswitchd.conf.db(5) for an
>>> options list.\n"
>>> -           "For COMMAND dump-flows, add-flow, mod-flow, del-flow
>>> and\n"
>>> -           "del-flows, DP is optional if there is only one
>>> datapath.\n",
>>> +           "For COMMAND dump-flows, add-flow, add-flows, mod-flow,
>>> del-flow\n"
>>> +           "and del-flows, DP is optional if there is only one
>>> datapath.\n",
>>>             program_name, program_name);
>>>      vlog_usage();
>>>      printf("\nOptions for show and mod-flow:\n"
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Eelco Chaudron Oct. 1, 2020, 1:01 p.m. UTC | #4
On 30 Sep 2020, at 15:06, Aaron Conole wrote:

> "Eelco Chaudron" <echaudro@redhat.com> writes:
>
>> On 22 Sep 2020, at 15:21, Aaron Conole wrote:
>>
>>> Eelco Chaudron <echaudro@redhat.com> writes:
>>>
>>>> When you would like to add, modify, or delete a lot of flows in the
>>>> datapath, for example when you want to measure performance, adding
>>>> one flow at the time won't scale. This as it takes a decent amount
>>>> of time to set up the datapath connection.
>>>>
>>>> This new command is in-line with the same command available in
>>>> ovs-ofctl which allows the same thing, with the only difference 
>>>> that
>>>> we do not verify all lines before we start execution. This allows 
>>>> for
>>>> a continuous add/delete stream. For example with a command like 
>>>> this:
>>>>
>>>> python3 -c 'while True:
>>>>   for i in range(0, 1000):
>>>>     print("add
>>>> in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{})
>>>> 1".format(int(i / 256), i % 256))
>>>>   for i in range(0, 1000):
>>>>     print("delete
>>>> in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.{}.{})".format(int(i
>>>> / 256), i % 256))' \
>>>> |  sudo utilities/ovs-dpctl add-flows -
>>>>
>>>>
>>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>>>> ---
>>>
>>> It may be worth mentioning this in NEWS.
>>
>> Good suggestion, will send out a v2 later.
>>
>>>
>>> Also, 'ovs-ofctl add-flows' is subtly different - there is the
>>> possibility to do a single transaction with 'ovs-ofctl add-flows',
>>> but this corresponding command cannot do that.  I guess that might 
>>> be
>>> what is intended by:
>>>
>>>   All flow mods are executed in the order specified.
>>>
>>> in the manpage.
>>
>> Not sure what you mean with single transaction? The following does 
>> work:
>>
>>   echo "in_port(0),eth(),eth_type(0x800),ipv4(src=100.1.0.2) 0" |
>> ovs-dpctl add-flows test -
>
> IIRC, all the flow rules can be added as part of a single openflow
> transaction with ovs-ofctl.  So multiple flow rules will be committed 
> at
> exactly the same time (--bundle, etc).
>
> This command will only ever do 1 at a time.  That's okay, but maybe 
> it's
> worth pointing out this difference.

Thanks Aaron, yes this was on purpose. I’ve updated the help text to 
the following:

  All flow modifications are executed as individual transactions in the 
order specified.

Will sent out a v2 soon…

//Eelco
diff mbox series

Patch

diff --git a/lib/dpctl.c b/lib/dpctl.c
index db2b1f896..eba2bdfa2 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -52,6 +52,12 @@ 
 #include "openvswitch/ofp-flow.h"
 #include "openvswitch/ofp-port.h"
 
+enum {
+    DPCTL_FLOWS_ADD = 0,
+    DPCTL_FLOWS_DEL,
+    DPCTL_FLOWS_MOD
+};
+
 typedef int dpctl_command_handler(int argc, const char *argv[],
                                   struct dpctl_params *);
 struct dpctl_command {
@@ -1102,28 +1108,22 @@  out_free:
 }
 
 static int
-dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags,
-               struct dpctl_params *dpctl_p)
+dpctl_put_flow_dpif(struct dpif *dpif, const char *key_s,
+                    const char *actions_s,
+                    enum dpif_flow_put_flags flags,
+                    struct dpctl_params *dpctl_p)
 {
-    const char *key_s = argv[argc - 2];
-    const char *actions_s = argv[argc - 1];
     struct dpif_flow_stats stats;
     struct dpif_port dpif_port;
     struct dpif_port_dump port_dump;
     struct ofpbuf actions;
     struct ofpbuf key;
     struct ofpbuf mask;
-    struct dpif *dpif;
     ovs_u128 ufid;
     bool ufid_present;
     struct simap port_names;
     int n, error;
 
-    error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
-    if (error) {
-        return error;
-    }
-
     ufid_present = false;
     n = odp_ufid_from_string(key_s, &ufid);
     if (n < 0) {
@@ -1185,6 +1185,24 @@  out_freeactions:
 out_freekeymask:
     ofpbuf_uninit(&mask);
     ofpbuf_uninit(&key);
+    return error;
+}
+
+static int
+dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags,
+               struct dpctl_params *dpctl_p)
+{
+    struct dpif *dpif;
+    int error;
+
+    error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
+    if (error) {
+        return error;
+    }
+
+    error = dpctl_put_flow_dpif(dpif, argv[argc - 2], argv[argc - 1], flags,
+                                dpctl_p);
+
     dpif_close(dpif);
     return error;
 }
@@ -1258,25 +1276,20 @@  out:
 }
 
 static int
-dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p)
+dpctl_del_flow_dpif(struct dpif *dpif, const char *key_s,
+                    struct dpctl_params *dpctl_p)
 {
-    const char *key_s = argv[argc - 1];
     struct dpif_flow_stats stats;
     struct dpif_port dpif_port;
     struct dpif_port_dump port_dump;
     struct ofpbuf key;
     struct ofpbuf mask; /* To be ignored. */
-    struct dpif *dpif;
+
     ovs_u128 ufid;
     bool ufid_present;
     struct simap port_names;
     int n, error;
 
-    error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
-    if (error) {
-        return error;
-    }
-
     ufid_present = false;
     n = odp_ufid_from_string(key_s, &ufid);
     if (n < 0) {
@@ -1334,6 +1347,23 @@  out:
     ofpbuf_uninit(&mask);
     ofpbuf_uninit(&key);
     simap_destroy(&port_names);
+    return error;
+}
+
+static int
+dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p)
+{
+    const char *key_s = argv[argc - 1];
+    struct dpif *dpif;
+    int error;
+
+    error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
+    if (error) {
+        return error;
+    }
+
+    error = dpctl_del_flow_dpif(dpif, key_s, dpctl_p);
+
     dpif_close(dpif);
     return error;
 }
@@ -1356,6 +1386,118 @@  dpctl_del_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
     return error;
 }
 
+static int
+dpctl_parse_add_flows_line(struct ds *s, char **flow, char **action)
+{
+    int command = DPCTL_FLOWS_ADD;
+    const char *line = ds_cstr(s);
+    size_t len;
+
+    /* First figure out the command, or fallback to FLOWS_ADD. */
+    line += strspn(line, " \t\r\n");
+    len = strcspn(line, ", \t\r\n");
+
+    if (!strncmp(line, "add", len)) {
+         command = DPCTL_FLOWS_ADD;
+    } else if (!strncmp(line, "delete", len)) {
+        command = DPCTL_FLOWS_DEL;
+    } else if (!strncmp(line, "modify", len)) {
+        command = DPCTL_FLOWS_MOD;
+    } else {
+        len = 0;
+    }
+    line += len;
+
+    /* Isolate flow and action (for add/modify). */
+    line += strspn(line, " \t\r\n");
+    len = strcspn(line, " \t\r\n");
+
+    if (len == 0) {
+        *flow = NULL;
+        *action = NULL;
+        return command;
+    }
+
+    *flow = strndup(line, len);
+
+    line += len;
+    line += strspn(line, " \t\r\n");
+    if (strlen(line)) {
+        *action = xstrdup(line);
+    } else {
+        *action = NULL;
+    }
+
+    return command;
+}
+
+static int
+dpctl_add_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
+{
+    const char *file_name = argv[argc - 1];
+    int line_number = 0;
+    struct dpif *dpif;
+    struct ds line;
+    FILE *stream;
+    int error;
+
+    error = opt_dpif_open(argc, argv, dpctl_p, 4, &dpif);
+    if (error) {
+        return error;
+    }
+
+    stream = !strcmp(file_name, "-") ? stdin : fopen(file_name, "r");
+    if (!stream) {
+        error = errno;
+        dpctl_error(dpctl_p, error, "Opening file \"%s\" failed", file_name);
+        goto out_close_dpif;
+    }
+
+    ds_init(&line);
+    while (!ds_get_preprocessed_line(&line, stream, &line_number)) {
+        /* We do not process all the lines first and then execute the actions
+         * as we would like to take commands as a continuous stream of
+         * commands from stdin.
+         */
+        char *flow = NULL;
+        char *action = NULL;
+        int cmd = dpctl_parse_add_flows_line(&line, &flow, &action);
+
+        if ((!flow && !action)
+            || ((cmd == DPCTL_FLOWS_ADD || cmd == DPCTL_FLOWS_MOD) && !action)
+            || (cmd == DPCTL_FLOWS_DEL && action)) {
+            dpctl_error(dpctl_p, 0,
+                        "Failed parsing line number %u, skipped!",
+                        line_number);
+        } else {
+            switch (cmd) {
+            case DPCTL_FLOWS_ADD:
+                dpctl_put_flow_dpif(dpif, flow, action,
+                                    DPIF_FP_CREATE, dpctl_p);
+                break;
+            case DPCTL_FLOWS_MOD:
+                dpctl_put_flow_dpif(dpif, flow, action,
+                                    DPIF_FP_MODIFY, dpctl_p);
+                break;
+            case DPCTL_FLOWS_DEL:
+                dpctl_del_flow_dpif(dpif, flow, dpctl_p);
+                break;
+            }
+        }
+
+        free(flow);
+        free(action);
+    }
+
+    ds_destroy(&line);
+    if (stream != stdin) {
+        fclose(stream);
+    }
+out_close_dpif:
+    dpif_close(dpif);
+    return 0;
+}
+
 static int
 dpctl_help(int argc OVS_UNUSED, const char *argv[] OVS_UNUSED,
            struct dpctl_params *dpctl_p)
@@ -2506,6 +2648,7 @@  static const struct dpctl_command all_commands[] = {
     { "dump-flows", "[dp] [filter=..] [type=..]",
       0, 3, dpctl_dump_flows, DP_RO },
     { "add-flow", "[dp] flow actions", 2, 3, dpctl_add_flow, DP_RW },
+    { "add-flows", "[dp] file", 1, 2, dpctl_add_flows, DP_RW },
     { "mod-flow", "[dp] flow actions", 2, 3, dpctl_mod_flow, DP_RW },
     { "get-flow", "[dp] ufid", 1, 2, dpctl_get_flow, DP_RO },
     { "del-flow", "[dp] flow", 1, 2, dpctl_del_flow, DP_RW },
diff --git a/lib/dpctl.man b/lib/dpctl.man
index 727d1f7be..e384e00da 100644
--- a/lib/dpctl.man
+++ b/lib/dpctl.man
@@ -194,6 +194,17 @@  ovs-dpctl add-flow myDP \\
 .
 .RE
 .TP
+.DO "\*(DX\fBadd\-flows\fR [\fIdp\fR] \fIfile\fR"
+Reads flow entries from \fIfile\fR (or \fBstdin\fR if \fIfile\fR is
+\fB\-\fR) and adds each entry to the datapath.
+.
+Each flow specification (e.g., each line in \fIfile\fR) may start with
+\fBadd\fR, \fBmodify\fR, or \fBdelete\fR keyword to specify whether a
+flow is to be added, modified, or deleted. A flow specification without
+one of these keywords is treated as a flow add.  All flow mods are
+executed in the order specified.
+.
+.TP
 .DO "[\fB\-s\fR | \fB\-\-statistics\fR]" "\*(DX\fBdel\-flow\fR" "[\fIdp\fR] \fIflow\fR"
 Deletes the flow from \fIdp\fR's flow table that matches \fIflow\fR.
 If \fB\-s\fR or \fB\-\-statistics\fR is specified, then
diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
index 7d99607f4..3ff3f714c 100644
--- a/utilities/ovs-dpctl.c
+++ b/utilities/ovs-dpctl.c
@@ -191,6 +191,7 @@  usage(void *userdata OVS_UNUSED)
            "  show DP...               show basic info on each DP\n"
            "  dump-flows [DP]          display flows in DP\n"
            "  add-flow [DP] FLOW ACTIONS add FLOW with ACTIONS to DP\n"
+           "  add-flows [DP] FILE        add flows from FILE\n"
            "  mod-flow [DP] FLOW ACTIONS change FLOW actions to ACTIONS in DP\n"
            "  get-flow [DP] ufid:UFID    fetch flow corresponding to UFID\n"
            "  del-flow [DP] FLOW         delete FLOW from DP\n"
@@ -205,8 +206,8 @@  usage(void *userdata OVS_UNUSED)
            "Each IFACE on add-dp, add-if, and set-if may be followed by\n"
            "comma-separated options.  See ovs-dpctl(8) for syntax, or the\n"
            "Interface table in ovs-vswitchd.conf.db(5) for an options list.\n"
-           "For COMMAND dump-flows, add-flow, mod-flow, del-flow and\n"
-           "del-flows, DP is optional if there is only one datapath.\n",
+           "For COMMAND dump-flows, add-flow, add-flows, mod-flow, del-flow\n"
+           "and del-flows, DP is optional if there is only one datapath.\n",
            program_name, program_name);
     vlog_usage();
     printf("\nOptions for show and mod-flow:\n"