[ovs-dev,v4] utilities/ofctl: add-meters for save and restore

put dump-meters' result in one line so add-meters can handle.
save and restore meters when restart ovs.
bundle functions are not implemented in this patch.

Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>

---
v4:
code refactor according to comments

v3:
add '--oneline' option for dump-meter(s) command

v2:
fix failed testcases, as dump-meters format changes
---
 include/openvswitch/ofp-meter.h |   8 +-
 lib/ofp-meter.c                 | 103 +++++++++++-
 lib/ofp-print.c                 |   3 +-
 tests/dpif-netdev.at            |   9 +
 utilities/ovs-ofctl.8.in        |  25 ++-
 utilities/ovs-ofctl.c           | 286 ++++++++++++++++++++++++++++----
 utilities/ovs-save              |   8 +
 7 files changed, 397 insertions(+), 45 deletions(-)

On Wed, Mar 01, 2023 at 04:05:17PM +0800, Wan Junjie wrote:
> put dump-meters' result in one line so add-meters can handle.
> save and restore meters when restart ovs.
> bundle functions are not implemented in this patch.
> 
> Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
> 
> ---
> v4:
> code refactor according to comments
> 
> v3:
> add '--oneline' option for dump-meter(s) command
> 
> v2:
> fix failed testcases, as dump-meters format changes
> ---
>  include/openvswitch/ofp-meter.h |   8 +-
>  lib/ofp-meter.c                 | 103 +++++++++++-
>  lib/ofp-print.c                 |   3 +-
>  tests/dpif-netdev.at            |   9 +
>  utilities/ovs-ofctl.8.in        |  25 ++-
>  utilities/ovs-ofctl.c           | 286 ++++++++++++++++++++++++++++----
>  utilities/ovs-save              |   8 +
>  7 files changed, 397 insertions(+), 45 deletions(-)

Hi Wan Junjie,

thanks for your patch.

It is a longish patch.
It might be nice to break it up a bit.

> diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h
> index 6776eae87..1bc91464e 100644
> --- a/include/openvswitch/ofp-meter.h
> +++ b/include/openvswitch/ofp-meter.h
> @@ -61,7 +61,8 @@ int ofputil_decode_meter_config(struct ofpbuf *,
>                                  struct ofputil_meter_config *,
>                                  struct ofpbuf *bands);
>  void ofputil_format_meter_config(struct ds *,
> -                                 const struct ofputil_meter_config *);
> +                                 const struct ofputil_meter_config *,
> +                                 int);

I think that:

1. Function declarations should include parameter names.
2. bool would be a better type for the new oneline parameter

void ofputil_format_meter_config(struct ds *,
                                 const struct ofputil_meter_config *,
                                 bool oneline);


>  struct ofputil_meter_mod {
>      uint16_t command;
> @@ -79,6 +80,11 @@ char *parse_ofp_meter_mod_str(struct ofputil_meter_mod *, const char *string,
>      OVS_WARN_UNUSED_RESULT;
>  void ofputil_format_meter_mod(struct ds *, const struct ofputil_meter_mod *);
>  
> +char *parse_ofp_meter_mod_file(const char *file_name, int command,
> +                               struct ofputil_meter_mod **mms, size_t *n_mms,
> +                               enum ofputil_protocol *usable_protocols)
> +    OVS_WARN_UNUSED_RESULT;
> +
>  struct ofputil_meter_stats {
>      uint32_t meter_id;
>      uint32_t flow_count;
> diff --git a/lib/ofp-meter.c b/lib/ofp-meter.c
> index 9ea40a0bf..b94cb6a05 100644
> --- a/lib/ofp-meter.c
> +++ b/lib/ofp-meter.c
> @@ -15,6 +15,7 @@
>   */
>  
>  #include <config.h>
> +#include <errno.h>
>  #include "openvswitch/ofp-meter.h"
>  #include "byte-order.h"
>  #include "nx-match.h"
> @@ -57,7 +58,7 @@ void
>  ofputil_format_meter_band(struct ds *s, enum ofp13_meter_flags flags,
>                            const struct ofputil_meter_band *mb)
>  {
> -    ds_put_cstr(s, "\ntype=");
> +    ds_put_cstr(s, "type=");
>      switch (mb->type) {
>      case OFPMBT13_DROP:
>          ds_put_cstr(s, "drop");
> @@ -343,7 +344,7 @@ ofp_print_meter_flags(struct ds *s, enum ofp13_meter_flags flags)
>  
>  void
>  ofputil_format_meter_config(struct ds *s,
> -                            const struct ofputil_meter_config *mc)
> +                            const struct ofputil_meter_config *mc, int oneline)
>  {
>      uint16_t i;
>  
> @@ -354,9 +355,12 @@ ofputil_format_meter_config(struct ds *s,
>  
>      ds_put_cstr(s, "bands=");
>      for (i = 0; i < mc->n_bands; ++i) {
> +        ds_put_cstr(s, oneline > 0 ? " ": "\n");

I think that as oneline is a boolean this may be clearer:

        ds_put_cstr(s, oneline ? " ": "\n");

>          ofputil_format_meter_band(s, mc->flags, &mc->bands[i]);
>      }
> -    ds_put_char(s, '\n');

Likewise,

    if (!oneline) {

> +    if (oneline == 0) {
> +        ds_put_char(s, '\n');
> +    }
>  }
>  
>  static enum ofperr
> @@ -578,6 +582,24 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string,
>  
>      /* Meters require at least OF 1.3. */
>      *usable_protocols = OFPUTIL_P_OF13_UP;
> +    if (command == -2) {
> +        size_t len;
> +
> +        string += strspn(string, " \t\r\n");   /* Skip white space. */
> +        len = strcspn(string, ", \t\r\n"); /* Get length of the first token. */
> +
> +        if (!strncmp(string, "add", len)) {
> +            command = OFPMC13_ADD;
> +        } else if (!strncmp(string, "delete", len)) {
> +            command = OFPMC13_DELETE;
> +        } else if (!strncmp(string, "modify", len)) {
> +            command = OFPMC13_MODIFY;
> +        } else {
> +            len = 0;
> +            command = OFPMC13_ADD;
> +        }
> +        string += len;
> +    }
>  
>      switch (command) {
>      case -1:
> @@ -606,6 +628,11 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string,
>      mm->meter.n_bands = 0;
>      mm->meter.bands = NULL;
>  
> +    if (command == OFPMC13_DELETE && string[0] == '\0') {
> +        mm->meter.meter_id = OFPM13_ALL;
> +        return NULL;
> +    }
> +
>      if (fields & F_BANDS) {
>          band_str = strstr(string, "band");
>          if (!band_str) {
> @@ -805,5 +832,73 @@ ofputil_format_meter_mod(struct ds *s, const struct ofputil_meter_mod *mm)
>          ds_put_format(s, " cmd:%d ", mm->command);
>      }
>  
> -    ofputil_format_meter_config(s, &mm->meter);
> +    ofputil_format_meter_config(s, &mm->meter, 0);

nit: If the type of the oneline argument becomes bool, then:

    ofputil_format_meter_config(s, &mm->meter, false);

> +}
> +
> +/* If 'command' is given as -2, each line may start with a command name ("add",
> + * "modify", "delete").  A missing command name is treated as "add".
> + */
> +char * OVS_WARN_UNUSED_RESULT
> +parse_ofp_meter_mod_file(const char *file_name,
> +                         int command,
> +                         struct ofputil_meter_mod **mms, size_t *n_mms,
> +                         enum ofputil_protocol *usable_protocols)
> +{
> +    size_t allocated_mms;
> +    int line_number;
> +    FILE *stream;
> +    struct ds s;
> +
> +    *mms = NULL;
> +    *n_mms = 0;
> +
> +    stream = !strcmp(file_name, "-") ? stdin : fopen(file_name, "r");
> +    if (stream == NULL) {
> +        return xasprintf("%s: open failed (%s)",
> +                         file_name, ovs_strerror(errno));
> +    }
> +
> +    allocated_mms = *n_mms;
> +    ds_init(&s);
> +    line_number = 0;
> +    *usable_protocols = OFPUTIL_P_ANY;
> +    while (!ds_get_preprocessed_line(&s, stream, &line_number)) {
> +        enum ofputil_protocol usable;
> +        char *error;
> +
> +        if (*n_mms >= allocated_mms) {
> +            *mms = x2nrealloc(*mms, &allocated_mms, sizeof **mms);
> +        }
> +        error = parse_ofp_meter_mod_str(&(*mms)[ *n_mms], ds_cstr(&s), command,

nit: '[ *n_mms]' -> '[*n_mms]'

> +                                         &usable);
> +        if (error) {
> +            size_t i;
> +
> +            for (i = 0; i < *n_mms; i++) {
> +                if (mms[i]->meter.bands) {
> +                    free(mms[i]->meter.bands);
> +                }
> +            }
> +            free(*mms);
> +            *mms = NULL;
> +            *n_mms = 0;
> +
> +            ds_destroy(&s);
> +            if (stream != stdin) {
> +                fclose(stream);
> +            }
> +
> +            char *ret = xasprintf("%s:%d: %s", file_name, line_number, error);
> +            free(error);
> +            return ret;
> +        }
> +        *usable_protocols &= usable;
> +        *n_mms += 1;
> +    }
> +
> +    ds_destroy(&s);
> +    if (stream != stdin) {
> +        fclose(stream);
> +    }
> +    return NULL;
>  }
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index 874079b84..9a25f45ea 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -379,8 +379,9 @@ ofp_print_meter_config_reply(struct ds *s, const struct ofp_header *oh)
>          if (retval) {
>              break;
>          }
> +
>          ds_put_char(s, '\n');
> -        ofputil_format_meter_config(s, &mc);
> +        ofputil_format_meter_config(s, &mc, 0);

nit: If the type of the oneline argument becomes bool, then:

    ofputil_format_meter_config(s, &mm->meter, false);

>      }
>      ofpbuf_uninit(&bands);
>  
> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> index baab60a22..5382eed0b 100644
> --- a/tests/dpif-netdev.at
> +++ b/tests/dpif-netdev.at
> @@ -298,6 +298,15 @@ meter=2 kbps burst stats bands=
>  type=drop rate=1 burst_size=2
>  ])
>  
> +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 --oneline], [0], [dnl
> +meter=1 pktps burst stats bands= type=drop rate=1 burst_size=1
> +meter=2 kbps burst stats bands= type=drop rate=1 burst_size=2
> +])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 meter=1 --oneline], [0], [dnl
> +meter=1 pktps burst stats bands= type=drop rate=1 burst_size=1
> +])
> +

Perhaps it is good to also test the restore function,
if that is not already being done.

>  ovs-appctl time/warp 5000
>  for i in `seq 1 7`; do
>    AT_CHECK(
> diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> index 0a611b2ee..c44091906 100644
> --- a/utilities/ovs-ofctl.8.in
> +++ b/utilities/ovs-ofctl.8.in
> @@ -492,23 +492,35 @@ the \fBBridge\fR table).  For more information, see ``Q: What versions
>  of OpenFlow does Open vSwitch support?'' in the Open vSwitch FAQ.
>  .
>  .IP "\fBadd\-meter \fIswitch meter\fR"
> +.IQ "\fBadd\-meter \fIswitch - < file\fR"
>  Add a meter entry to \fIswitch\fR's tables. The \fImeter\fR syntax is
>  described in section \fBMeter Syntax\fR, below.
>  .
> +.IP "\fBadd\-meters \fIswitch file\fR"
> +Add each meter entry to \fIswitch\fR's tables. Each meter specification
> +(each line in file) may start with \fBadd\fI, \fBmodify\fI or \fBdelete\fI
> +keyword to specify whether a meter is to be added, modified, or deleted.
> +For backwards compatibility a meter specification without one of these keywords
> +is treated as a meter add. The \fImeter\fR syntax is described in section
> +\fBMeter Syntax\fR, below.
> +.
>  .IP "\fBmod\-meter \fIswitch meter\fR"
> +.IQ "\fBmod\-meter \fIswitch - < file\fR"
>  Modify an existing meter.
>  .
>  .IP "\fBdel\-meters \fIswitch\fR [\fImeter\fR]"
> +.IQ "\fBdel\-meters \fIswitch\fR - < file"
>  Delete entries from \fIswitch\fR's meter table.  To delete only a
> -specific meter, specify its number as \fImeter\fR.  Otherwise, if
> +specific meter, specify a meter syntax \fBmeter=\fIid\fR.  Otherwise, if

Are there backwards compatibility concerns here...

>  \fImeter\fR is omitted, or if it is specified as \fBall\fR, then all
>  meters are deleted.
>  .
> -.IP "\fBdump\-meters \fIswitch\fR [\fImeter\fR]"
> +.IP "[\fB\-\-oneline\fR] \fBdump\-meters \fIswitch\fR [\fImeter\fR]"
>  Print entries from \fIswitch\fR's meter table.  To print only a
> -specific meter, specify its number as \fImeter\fR.  Otherwise, if
> +specific meter, specify a meter syntax \fBmeter=\fIid\fR.  Otherwise, if

... and here?

>  \fImeter\fR is omitted, or if it is specified as \fBall\fR, then all
> -meters are printed.
> +meters are printed.  With \fB\-\-oneline\fR, band information will be
> +combined into one line.
>  .
>  .IP "\fBmeter\-stats \fIswitch\fR [\fImeter\fR]"
>  Print meter statistics.  \fImeter\fR can specify a single meter with
> @@ -1342,6 +1354,11 @@ includes flow duration, packet and byte counts, and idle and hard age
>  in its output.  With \fB\-\-no\-stats\fR, it omits all of these, as
>  well as cookie values and table IDs if they are zero.
>  .
> +.IP "\fB\-\-oneline\fR"
> +The \fBdump\-meters\fR command prints each band in a separate line by
> +default. With \fB\-\-oneline\fR, all bands will be printed in a single
> +line. This is useful for dumping meters to a file and restoring them.
> +.
>  .IP "\fB\-\-read-only\fR"
>  Do not execute read/write commands.
>  .
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index eabec18a3..b9578d682 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -157,6 +157,9 @@ static int print_pcap = 0;
>  /* --raw: Makes "ofp-print" read binary data from stdin. */
>  static int raw = 0;
>  
> +/* --oneline: show meter config in a single line. */
> +static int oneline = 0;
> +

nit: bool would be a better type for 'online'.
     Likewise, there could be a cleanup patch for similar variables
     that are currently of type int.

>  static const struct ovs_cmdl_command *get_all_commands(void);
>  
>  OVS_NO_RETURN static void usage(void);
> @@ -245,6 +248,7 @@ parse_options(int argc, char *argv[])
>          {"pcap", no_argument, &print_pcap, 1},
>          {"raw", no_argument, &raw, 1},
>          {"read-only", no_argument, NULL, OPT_READ_ONLY},
> +        {"oneline", no_argument, &oneline, 1},
>          DAEMON_LONG_OPTIONS,
>          OFP_VERSION_LONG_OPTIONS,
>          VLOG_LONG_OPTIONS,
> @@ -475,9 +479,10 @@ usage(void)
>             "  dump-group-stats SWITCH [GROUP]  print group statistics\n"
>             "  queue-get-config SWITCH [PORT]  print queue config for PORT\n"
>             "  add-meter SWITCH METER      add meter described by METER\n"
> +           "  add-meters SWITCH FILE      add meters from FILE\n"
>             "  mod-meter SWITCH METER      modify specific METER\n"
>             "  del-meters SWITCH [METER]   delete meters matching METER\n"
> -           "  dump-meters SWITCH [METER]  print METER configuration\n"
> +           "  dump-meters SWITCH [METER]  print METER entries\n"
>             "  meter-stats SWITCH [METER]  print meter statistics\n"
>             "  meter-features SWITCH       print meter features\n"
>             "  add-tlv-map SWITCH MAP      add TLV option MAPpings\n"
> @@ -515,6 +520,7 @@ usage(void)
>             "  --rsort[=field]             sort in descending order\n"
>             "  --names                     show port names instead of numbers\n"
>             "  --no-names                  show port numbers, but not names\n"
> +           "  --oneline                   show meter bands in a single line\n"
>             "  --unixctl=SOCKET            set control socket name\n"
>             "  --color[=always|never|auto] control use of color in output\n"
>             "  -h, --help                  display this help message\n"
> @@ -1443,7 +1449,7 @@ set_protocol_for_flow_dump(struct vconn *vconn,
>      if (usable_protocols & allowed_protocols) {
>          ovs_fatal(0, "switch does not support any of the usable flow "
>                    "formats (%s)", usable_s);
> -} else {
> +    } else {
>          char *allowed_s = ofputil_protocols_to_string(allowed_protocols);
>          ovs_fatal(0, "none of the usable flow formats (%s) is among the "
>                    "allowed flow formats (%s)", usable_s, allowed_s);
> @@ -4080,57 +4086,107 @@ ofctl_diff_flows(struct ovs_cmdl_context *ctx)
>  }
>  
>  static void
> -ofctl_meter_mod__(const char *bridge, const char *str, int command)
> +ofctl_meter_mod__(const char *remote, struct ofputil_meter_mod *mms,
> +                  size_t n_mms, enum ofputil_protocol usable_protocols)
>  {
> -    struct ofputil_meter_mod mm;
> +    struct ofputil_meter_mod *mm;
>      struct vconn *vconn;
>      enum ofputil_protocol protocol;
> -    enum ofputil_protocol usable_protocols;
>      enum ofp_version version;
> +    struct ofpbuf *request;
> +    size_t i;
>  
> -    memset(&mm, 0, sizeof mm);
> -    if (str) {
> -        char *error;
> -        error = parse_ofp_meter_mod_str(&mm, str, command, &usable_protocols);
> -        if (error) {
> -            ovs_fatal(0, "%s", error);
> -        }
> -    } else {
> -        usable_protocols = OFPUTIL_P_OF13_UP;
> -        mm.command = command;
> -        mm.meter.meter_id = OFPM13_ALL;
> +    protocol = open_vconn_for_flow_mod(remote, &vconn, usable_protocols);
> +    version = ofputil_protocol_to_ofp_version(protocol);
> +
> +     for (i = 0; i < n_mms; i++) {
> +        mm = &mms[i];
> +        request = ofputil_encode_meter_mod(version, mm);
> +        transact_noreply(vconn, request);
> +        free(mm->meter.bands);
>      }
>  
> -    protocol = open_vconn_for_flow_mod(bridge, &vconn, usable_protocols);
> -    version = ofputil_protocol_to_ofp_version(protocol);
> -    transact_noreply(vconn, ofputil_encode_meter_mod(version, &mm));
> -    free(mm.meter.bands);
>      vconn_close(vconn);
>  }
>  
>  static void
> -ofctl_meter_request__(const char *bridge, const char *str,
> -                      enum ofputil_meter_request_type type)
> +ofctl_meter_mod_file(int argc OVS_UNUSED, char *argv[], int command)
> +{
> +    enum ofputil_protocol usable_protocols;
> +    struct ofputil_meter_mod *mms = NULL;
> +    size_t n_mms = 0;
> +    char *error;
> +
> +    if (command == OFPMC13_ADD) {
> +        /* Allow the file to specify a mix of commands. If none specified at
> +         * the beginning of any given line, then the default is OFPMC13_ADD, so
> +         * this is backwards compatible. */
> +        command = -2;
> +    }
> +    error = parse_ofp_meter_mod_file(argv[2], command,
> +                                     &mms, &n_mms, &usable_protocols);
> +    if (error) {
> +        ovs_fatal(0, "%s", error);
> +    }
> +    ofctl_meter_mod__(argv[1], mms, n_mms, usable_protocols);
> +    free(mms);
> +}
> +
> +static void
> +ofctl_meter_mod(int argc, char *argv[], uint16_t command)
> +{
> +    if (argc > 2 && !strcmp(argv[2], "-")) {
> +        ofctl_meter_mod_file(argc, argv, command);
> +    } else {
> +        enum ofputil_protocol usable_protocols;
> +        struct ofputil_meter_mod mm;
> +        char *error;
> +        memset(&mm, 0, sizeof mm);
> +        error = parse_ofp_meter_mod_str(&mm, argc > 2 ? argv[2] : "", command,
> +                                        &usable_protocols);
> +        if (error) {
> +            ovs_fatal(0, "%s", error);
> +        }
> +        ofctl_meter_mod__(argv[1], &mm, 1, usable_protocols);
> +    }
> +}

nit: blank line

> +static struct vconn *
> +prepare_dump_meters(const char *bridge, const char *str,
> +                    struct ofputil_meter_mod *mm,
> +                    enum ofputil_protocol *protocolp)
>  {
> -    struct ofputil_meter_mod mm;
>      struct vconn *vconn;
>      enum ofputil_protocol usable_protocols;
>      enum ofputil_protocol protocol;
> -    enum ofp_version version;
>  
> -    memset(&mm, 0, sizeof mm);
>      if (str) {
>          char *error;
> -        error = parse_ofp_meter_mod_str(&mm, str, -1, &usable_protocols);
> +        error = parse_ofp_meter_mod_str(mm, str, -1, &usable_protocols);
>          if (error) {
>              ovs_fatal(0, "%s", error);
>          }
>      } else {
>          usable_protocols = OFPUTIL_P_OF13_UP;
> -        mm.meter.meter_id = OFPM13_ALL;
> +        mm->meter.meter_id = OFPM13_ALL;
>      }
>  
> -    protocol = open_vconn_for_flow_mod(bridge, &vconn, usable_protocols);
> +    protocol = open_vconn(bridge, &vconn);
> +    *protocolp = set_protocol_for_flow_dump(vconn, protocol, usable_protocols);
> +    return vconn;
> +}
> +
> +
> +static void
> +ofctl_meter_request__(const char *bridge, const char *str,
> +                      enum ofputil_meter_request_type type)
> +{
> +    struct ofputil_meter_mod mm;
> +    enum ofputil_protocol protocol;
> +    struct vconn *vconn;
> +    enum ofp_version version;
> +
> +    memset(&mm, 0, sizeof mm);
> +    vconn = prepare_dump_meters(bridge, str, &mm, &protocol);
>      version = ofputil_protocol_to_ofp_version(protocol);
>      dump_transaction(vconn, ofputil_encode_meter_request(version, type,
>                                                           mm.meter.meter_id));
> @@ -4138,32 +4194,190 @@ ofctl_meter_request__(const char *bridge, const char *str,
>      vconn_close(vconn);
>  }
>  
> -
>  static void
>  ofctl_add_meter(struct ovs_cmdl_context *ctx)
>  {
> -    ofctl_meter_mod__(ctx->argv[1], ctx->argv[2], OFPMC13_ADD);
> +    ofctl_meter_mod(ctx->argc, ctx->argv, OFPMC13_ADD);
> +}
> +
> +static void
> +ofctl_add_meters(struct ovs_cmdl_context *ctx)
> +{
> +    ofctl_meter_mod_file(ctx->argc, ctx->argv, OFPMC13_ADD);
>  }
>  
>  static void
>  ofctl_mod_meter(struct ovs_cmdl_context *ctx)
>  {
> -    ofctl_meter_mod__(ctx->argv[1], ctx->argv[2], OFPMC13_MODIFY);
> +    ofctl_meter_mod(ctx->argc, ctx->argv, OFPMC13_MODIFY);
>  }
>  
>  static void
>  ofctl_del_meters(struct ovs_cmdl_context *ctx)
>  {
> -    ofctl_meter_mod__(ctx->argv[1], ctx->argc > 2 ? ctx->argv[2] : NULL, OFPMC13_DELETE);
> +    ofctl_meter_mod(ctx->argc, ctx->argv, OFPMC13_DELETE);
>  }
>  
>  static void
> -ofctl_dump_meters(struct ovs_cmdl_context *ctx)
> +ofctl_dump_meters__(struct ovs_cmdl_context *ctx)
>  {
>      ofctl_meter_request__(ctx->argv[1], ctx->argc > 2 ? ctx->argv[2] : NULL,
>                            OFPUTIL_METER_CONFIG);
>  }
>  
> +static int
> +recv_meter_stats_reply(struct vconn *vconn, ovs_be32 send_xid,
> +                       struct ofpbuf **replyp,
> +                       struct ofputil_meter_config *mc, struct ofpbuf *bands)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> +    struct ofpbuf *reply = *replyp;
> +
> +    for (;;) {
> +        int retval;
> +        bool more;
> +
> +        if (!reply) {
> +            enum ofptype type;
> +            enum ofperr error;
> +
> +            do {
> +                error = vconn_recv_block(vconn, &reply);
> +                if (error) {
> +                    return error;
> +                }
> +            } while (((struct ofp_header *) reply->data)->xid != send_xid);
> +
> +            error = ofptype_decode(&type, reply->data);
> +            if (error || type != OFPTYPE_METER_CONFIG_STATS_REPLY) {
> +                VLOG_WARN_RL(&rl, "received bad reply: %s",
> +                             ofp_to_string(reply->data, reply->size,
> +                                           NULL, NULL, 1));
> +                return EPROTO;
> +            }
> +        }
> +
> +        /* Pull an individual meter reply out of the message. */
> +        retval = ofputil_decode_meter_config(reply, mc, bands);
> +        switch (retval) {
> +        case 0:
> +            *replyp = reply;
> +            return 0;
> +
> +        case EOF:
> +            more = ofpmp_more(reply->header);
> +            ofpbuf_delete(reply);
> +            reply = NULL;
> +            if (!more) {
> +                *replyp = NULL;
> +                return EOF;
> +            }
> +            break;
> +
> +        default:
> +            VLOG_WARN_RL(&rl, "parse error in reply (%s)",
> +                         ofperr_to_string(retval));
> +            return EPROTO;
> +        }
> +    }
> +}
> +
> +static int
> +vconn_dump_meters(struct vconn *vconn,
> +                 struct ofpbuf *request,
> +                 struct ofputil_meter_config **mcsp, size_t *n_mcsp)
> +{
> +    struct ofputil_meter_config *mcs = NULL;
> +    size_t n_mcs = 0;
> +    size_t allocated_mcs = 0;
> +
> +    const struct ofp_header *oh = request->data;
> +    ovs_be32 send_xid = oh->xid;
> +    int error = vconn_send_block(vconn, request);
> +    if (error) {
> +        goto exit;
> +    }
> +
> +    struct ofpbuf *reply = NULL;
> +    struct ofpbuf bands;
> +    ofpbuf_init(&bands, 64);
> +    for (;;) {
> +        if (n_mcs >= allocated_mcs) {
> +            mcs = x2nrealloc(mcs, &allocated_mcs, sizeof *mcs);
> +        }
> +
> +        struct ofputil_meter_config *mc = &mcs[n_mcs];
> +        error = recv_meter_stats_reply(vconn, send_xid, &reply, mc, &bands);
> +        if (error) {
> +            if (error == EOF) {
> +                error = 0;
> +            }
> +            break;
> +        }
> +        mc->bands = xmemdup(mc->bands, mc->n_bands * sizeof(mc->bands[0]));
> +        n_mcs++;
> +    }
> +    ofpbuf_uninit(&bands);
> +    ofpbuf_delete(reply);
> +
> +    if (error) {
> +        for (size_t i = 0; i < n_mcs; i++) {
> +            free(CONST_CAST(struct ofputil_meter_band *, mcs[i].bands));
> +        }
> +        free(mcs);
> +
> +        mcs = NULL;
> +        n_mcs = 0;
> +    }
> +
> +exit:
> +    *mcsp = mcs;
> +    *n_mcsp = n_mcs;
> +    return error;
> +}
> +
> +static void
> +ofctl_dump_meters(struct ovs_cmdl_context *ctx)
> +{
> +    if (!oneline) {
> +        ofctl_dump_meters__(ctx);
> +    } else {
> +        struct ofputil_meter_mod mm;
> +        struct vconn *vconn;
> +        enum ofputil_protocol protocol;
> +        enum ofp_version version;
> +
> +        memset(&mm, 0, sizeof mm);
> +        const char *bridge = ctx->argv[1];
> +        const char *str = ctx->argc > 2 ? ctx->argv[2] : NULL;
> +
> +        vconn = prepare_dump_meters(bridge, str, &mm, &protocol);
> +        version = ofputil_protocol_to_ofp_version(protocol);
> +        struct ofpbuf *request = ofputil_encode_meter_request(version,
> +                        OFPUTIL_METER_CONFIG, mm.meter.meter_id);

The logic in the two 'paragraphs' above seems to largely duplicate
ofctl_dump_meters__() => ofctl_meter_request__().

Could those functions be parameterised over oneline?

> +
> +        struct ofputil_meter_config *mcs;
> +        size_t n_mtrs;
> +        run(vconn_dump_meters(vconn, request, &mcs, &n_mtrs),
> +            "dump meters");
> +
> +        struct ds s = DS_EMPTY_INITIALIZER;
> +        for (size_t i = 0; i < n_mtrs; i ++) {
> +            ds_clear(&s);
> +            ofputil_format_meter_config(&s, &mcs[i], 1);
> +            printf("%s\n", ds_cstr(&s));
> +        }
> +        ds_destroy(&s);
> +
> +        for (size_t i = 0; i < n_mtrs; i ++) {
> +            free(CONST_CAST(struct ofputil_meter_band *, mcs[i].bands));
> +        } 
> +        free(mcs);

The above, ofputil_format_meter_config(), and recv_meter_stats_reply()
seems to be a lot of code to customise what was otherwise a call to
dump_transaction().

Perhaps it would be cleaner to parameterise ofp_print() over 'oneline'.


> +        free(mm.meter.bands);
> +        vconn_close(vconn);
> +    }
> +}
> +
>  static void
>  ofctl_meter_stats(struct ovs_cmdl_context *ctx)
>  {
> @@ -5068,15 +5282,17 @@ static const struct ovs_cmdl_command all_commands[] = {
>        2, 2, ofctl_diff_flows, OVS_RW },
>      { "add-meter", "switch meter",
>        2, 2, ofctl_add_meter, OVS_RW },
> +    { "add-meters", "switch file",
> +      2, 2, ofctl_add_meters, OVS_RW },
>      { "mod-meter", "switch meter",
>        2, 2, ofctl_mod_meter, OVS_RW },
> -    { "del-meter", "switch meter",
> +    { "del-meter", "switch [meter]",
>        1, 2, ofctl_del_meters, OVS_RW },
>      { "del-meters", "switch",
>        1, 2, ofctl_del_meters, OVS_RW },
>      { "dump-meter", "switch meter",
>        1, 2, ofctl_dump_meters, OVS_RO },
> -    { "dump-meters", "switch",
> +    { "dump-meters", "switch [meter]",
>        1, 2, ofctl_dump_meters, OVS_RO },
>      { "meter-stats", "switch [meter]",
>        1, 2, ofctl_meter_stats, OVS_RO },
> diff --git a/utilities/ovs-save b/utilities/ovs-save
> index 67092ecf7..d1baa3415 100755
> --- a/utilities/ovs-save
> +++ b/utilities/ovs-save
> @@ -139,6 +139,9 @@ save_flows () {
>          echo "ovs-ofctl -O $ofp_version add-groups ${bridge} \
>                \"$workdir/$bridge.groups.dump\" ${bundle}"
>  
> +        echo "ovs-ofctl -O $ofp_version add-meters ${bridge} \
> +              \"$workdir/$bridge.meters.dump\""
> +
>          echo "ovs-ofctl -O $ofp_version replace-flows ${bridge} \
>                \"$workdir/$bridge.flows.dump\" ${bundle}"
>  
> @@ -147,6 +150,11 @@ save_flows () {
>                  -e '/^NXST_GROUP_DESC/d' > \
>                  "$workdir/$bridge.groups.dump"
>  
> +        ovs-ofctl -O $ofp_version dump-meters "$bridge" --oneline | \
> +            sed -e '/^OFPST_METER_CONFIG/d' \
> +                -e '/^NXST_METER_CONFIG/d' > \
> +                "$workdir/$bridge.meters.dump"
> +
>          ovs-ofctl -O $ofp_version dump-flows --no-names --no-stats "$bridge" | \
>              sed -e '/NXST_FLOW/d' \
>                  -e '/OFPST_FLOW/d' \
> -- 
> 2.39.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Hi Simon,

Thanks for the review.

On Fri, Mar 3, 2023 at 11:06 PM Simon Horman <simon.horman@corigine.com> wrote:
>
> On Wed, Mar 01, 2023 at 04:05:17PM +0800, Wan Junjie wrote:
> > put dump-meters' result in one line so add-meters can handle.
> > save and restore meters when restart ovs.
> > bundle functions are not implemented in this patch.
> >
> > Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
> >
> > ---
> > v4:
> > code refactor according to comments
> >
> > v3:
> > add '--oneline' option for dump-meter(s) command
> >
> > v2:
> > fix failed testcases, as dump-meters format changes
> > ---
> >  include/openvswitch/ofp-meter.h |   8 +-
> >  lib/ofp-meter.c                 | 103 +++++++++++-
> >  lib/ofp-print.c                 |   3 +-
> >  tests/dpif-netdev.at            |   9 +
> >  utilities/ovs-ofctl.8.in        |  25 ++-
> >  utilities/ovs-ofctl.c           | 286 ++++++++++++++++++++++++++++----
> >  utilities/ovs-save              |   8 +
> >  7 files changed, 397 insertions(+), 45 deletions(-)
>
> Hi Wan Junjie,
>
> thanks for your patch.
>
> It is a longish patch.
> It might be nice to break it up a bit.
>

> > diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h
> > index 6776eae87..1bc91464e 100644
> > --- a/include/openvswitch/ofp-meter.h
> > +++ b/include/openvswitch/ofp-meter.h
> > @@ -61,7 +61,8 @@ int ofputil_decode_meter_config(struct ofpbuf *,
> >                                  struct ofputil_meter_config *,
> >                                  struct ofpbuf *bands);
> >  void ofputil_format_meter_config(struct ds *,
> > -                                 const struct ofputil_meter_config *);
> > +                                 const struct ofputil_meter_config *,
> > +                                 int);
>
> I think that:
>
> 1. Function declarations should include parameter names.
> 2. bool would be a better type for the new oneline parameter
>
> void ofputil_format_meter_config(struct ds *,
>                                  const struct ofputil_meter_config *,
>                                  bool oneline);
>
OK.

>
> >  struct ofputil_meter_mod {
> >      uint16_t command;
> > @@ -79,6 +80,11 @@ char *parse_ofp_meter_mod_str(struct ofputil_meter_mod *, const char *string,
> >      OVS_WARN_UNUSED_RESULT;
> >  void ofputil_format_meter_mod(struct ds *, const struct ofputil_meter_mod *);
> >
> > +char *parse_ofp_meter_mod_file(const char *file_name, int command,
> > +                               struct ofputil_meter_mod **mms, size_t *n_mms,
> > +                               enum ofputil_protocol *usable_protocols)
> > +    OVS_WARN_UNUSED_RESULT;
> > +
> >  struct ofputil_meter_stats {
> >      uint32_t meter_id;
> >      uint32_t flow_count;
> > diff --git a/lib/ofp-meter.c b/lib/ofp-meter.c
> > index 9ea40a0bf..b94cb6a05 100644
> > --- a/lib/ofp-meter.c
> > +++ b/lib/ofp-meter.c
> > @@ -15,6 +15,7 @@
> >   */
> >
> >  #include <config.h>
> > +#include <errno.h>
> >  #include "openvswitch/ofp-meter.h"
> >  #include "byte-order.h"
> >  #include "nx-match.h"
> > @@ -57,7 +58,7 @@ void
> >  ofputil_format_meter_band(struct ds *s, enum ofp13_meter_flags flags,
> >                            const struct ofputil_meter_band *mb)
> >  {
> > -    ds_put_cstr(s, "\ntype=");
> > +    ds_put_cstr(s, "type=");
> >      switch (mb->type) {
> >      case OFPMBT13_DROP:
> >          ds_put_cstr(s, "drop");
> > @@ -343,7 +344,7 @@ ofp_print_meter_flags(struct ds *s, enum ofp13_meter_flags flags)
> >
> >  void
> >  ofputil_format_meter_config(struct ds *s,
> > -                            const struct ofputil_meter_config *mc)
> > +                            const struct ofputil_meter_config *mc, int oneline)
> >  {
> >      uint16_t i;
> >
> > @@ -354,9 +355,12 @@ ofputil_format_meter_config(struct ds *s,
> >
> >      ds_put_cstr(s, "bands=");
> >      for (i = 0; i < mc->n_bands; ++i) {
> > +        ds_put_cstr(s, oneline > 0 ? " ": "\n");
>
> I think that as oneline is a boolean this may be clearer:
>
>         ds_put_cstr(s, oneline ? " ": "\n");
>
OK

> >          ofputil_format_meter_band(s, mc->flags, &mc->bands[i]);
> >      }
> > -    ds_put_char(s, '\n');
>
> Likewise,
>
>     if (!oneline) {
>
> > +    if (oneline == 0) {
> > +        ds_put_char(s, '\n');
> > +    }
> >  }
> >
> >  static enum ofperr
> > @@ -578,6 +582,24 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string,
> >
> >      /* Meters require at least OF 1.3. */
> >      *usable_protocols = OFPUTIL_P_OF13_UP;
> > +    if (command == -2) {
> > +        size_t len;
> > +
> > +        string += strspn(string, " \t\r\n");   /* Skip white space. */
> > +        len = strcspn(string, ", \t\r\n"); /* Get length of the first token. */
> > +
> > +        if (!strncmp(string, "add", len)) {
> > +            command = OFPMC13_ADD;
> > +        } else if (!strncmp(string, "delete", len)) {
> > +            command = OFPMC13_DELETE;
> > +        } else if (!strncmp(string, "modify", len)) {
> > +            command = OFPMC13_MODIFY;
> > +        } else {
> > +            len = 0;
> > +            command = OFPMC13_ADD;
> > +        }
> > +        string += len;
> > +    }
> >
> >      switch (command) {
> >      case -1:
> > @@ -606,6 +628,11 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string,
> >      mm->meter.n_bands = 0;
> >      mm->meter.bands = NULL;
> >
> > +    if (command == OFPMC13_DELETE && string[0] == '\0') {
> > +        mm->meter.meter_id = OFPM13_ALL;
> > +        return NULL;
> > +    }
> > +
> >      if (fields & F_BANDS) {
> >          band_str = strstr(string, "band");
> >          if (!band_str) {
> > @@ -805,5 +832,73 @@ ofputil_format_meter_mod(struct ds *s, const struct ofputil_meter_mod *mm)
> >          ds_put_format(s, " cmd:%d ", mm->command);
> >      }
> >
> > -    ofputil_format_meter_config(s, &mm->meter);
> > +    ofputil_format_meter_config(s, &mm->meter, 0);
>
> nit: If the type of the oneline argument becomes bool, then:
>
>     ofputil_format_meter_config(s, &mm->meter, false);
>
> > +}
> > +
> > +/* If 'command' is given as -2, each line may start with a command name ("add",
> > + * "modify", "delete").  A missing command name is treated as "add".
> > + */
> > +char * OVS_WARN_UNUSED_RESULT
> > +parse_ofp_meter_mod_file(const char *file_name,
> > +                         int command,
> > +                         struct ofputil_meter_mod **mms, size_t *n_mms,
> > +                         enum ofputil_protocol *usable_protocols)
> > +{
> > +    size_t allocated_mms;
> > +    int line_number;
> > +    FILE *stream;
> > +    struct ds s;
> > +
> > +    *mms = NULL;
> > +    *n_mms = 0;
> > +
> > +    stream = !strcmp(file_name, "-") ? stdin : fopen(file_name, "r");
> > +    if (stream == NULL) {
> > +        return xasprintf("%s: open failed (%s)",
> > +                         file_name, ovs_strerror(errno));
> > +    }
> > +
> > +    allocated_mms = *n_mms;
> > +    ds_init(&s);
> > +    line_number = 0;
> > +    *usable_protocols = OFPUTIL_P_ANY;
> > +    while (!ds_get_preprocessed_line(&s, stream, &line_number)) {
> > +        enum ofputil_protocol usable;
> > +        char *error;
> > +
> > +        if (*n_mms >= allocated_mms) {
> > +            *mms = x2nrealloc(*mms, &allocated_mms, sizeof **mms);
> > +        }
> > +        error = parse_ofp_meter_mod_str(&(*mms)[ *n_mms], ds_cstr(&s), command,
>
> nit: '[ *n_mms]' -> '[*n_mms]'
>
OK

> > +                                         &usable);
> > +        if (error) {
> > +            size_t i;
> > +
> > +            for (i = 0; i < *n_mms; i++) {
> > +                if (mms[i]->meter.bands) {
> > +                    free(mms[i]->meter.bands);
> > +                }
> > +            }
> > +            free(*mms);
> > +            *mms = NULL;
> > +            *n_mms = 0;
> > +
> > +            ds_destroy(&s);
> > +            if (stream != stdin) {
> > +                fclose(stream);
> > +            }
> > +
> > +            char *ret = xasprintf("%s:%d: %s", file_name, line_number, error);
> > +            free(error);
> > +            return ret;
> > +        }
> > +        *usable_protocols &= usable;
> > +        *n_mms += 1;
> > +    }
> > +
> > +    ds_destroy(&s);
> > +    if (stream != stdin) {
> > +        fclose(stream);
> > +    }
> > +    return NULL;
> >  }
> > diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> > index 874079b84..9a25f45ea 100644
> > --- a/lib/ofp-print.c
> > +++ b/lib/ofp-print.c
> > @@ -379,8 +379,9 @@ ofp_print_meter_config_reply(struct ds *s, const struct ofp_header *oh)
> >          if (retval) {
> >              break;
> >          }
> > +
> >          ds_put_char(s, '\n');
> > -        ofputil_format_meter_config(s, &mc);
> > +        ofputil_format_meter_config(s, &mc, 0);
>
> nit: If the type of the oneline argument becomes bool, then:
>
>     ofputil_format_meter_config(s, &mm->meter, false);
>
OK

> >      }
> >      ofpbuf_uninit(&bands);
> >
> > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> > index baab60a22..5382eed0b 100644
> > --- a/tests/dpif-netdev.at
> > +++ b/tests/dpif-netdev.at
> > @@ -298,6 +298,15 @@ meter=2 kbps burst stats bands=
> >  type=drop rate=1 burst_size=2
> >  ])
> >
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 --oneline], [0], [dnl
> > +meter=1 pktps burst stats bands= type=drop rate=1 burst_size=1
> > +meter=2 kbps burst stats bands= type=drop rate=1 burst_size=2
> > +])
> > +
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 meter=1 --oneline], [0], [dnl
> > +meter=1 pktps burst stats bands= type=drop rate=1 burst_size=1
> > +])
> > +
>
> Perhaps it is good to also test the restore function,
> if that is not already being done.
>
OK

> >  ovs-appctl time/warp 5000
> >  for i in `seq 1 7`; do
> >    AT_CHECK(
> > diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> > index 0a611b2ee..c44091906 100644
> > --- a/utilities/ovs-ofctl.8.in
> > +++ b/utilities/ovs-ofctl.8.in
> > @@ -492,23 +492,35 @@ the \fBBridge\fR table).  For more information, see ``Q: What versions
> >  of OpenFlow does Open vSwitch support?'' in the Open vSwitch FAQ.
> >  .
> >  .IP "\fBadd\-meter \fIswitch meter\fR"
> > +.IQ "\fBadd\-meter \fIswitch - < file\fR"
> >  Add a meter entry to \fIswitch\fR's tables. The \fImeter\fR syntax is
> >  described in section \fBMeter Syntax\fR, below.
> >  .
> > +.IP "\fBadd\-meters \fIswitch file\fR"
> > +Add each meter entry to \fIswitch\fR's tables. Each meter specification
> > +(each line in file) may start with \fBadd\fI, \fBmodify\fI or \fBdelete\fI
> > +keyword to specify whether a meter is to be added, modified, or deleted.
> > +For backwards compatibility a meter specification without one of these keywords
> > +is treated as a meter add. The \fImeter\fR syntax is described in section
> > +\fBMeter Syntax\fR, below.
> > +.
> >  .IP "\fBmod\-meter \fIswitch meter\fR"
> > +.IQ "\fBmod\-meter \fIswitch - < file\fR"
> >  Modify an existing meter.
> >  .
> >  .IP "\fBdel\-meters \fIswitch\fR [\fImeter\fR]"
> > +.IQ "\fBdel\-meters \fIswitch\fR - < file"
> >  Delete entries from \fIswitch\fR's meter table.  To delete only a
> > -specific meter, specify its number as \fImeter\fR.  Otherwise, if
> > +specific meter, specify a meter syntax \fBmeter=\fIid\fR.  Otherwise, if
>
> Are there backwards compatibility concerns here...
>
In fact, old docs are not clear on meter syntax, it means meter=id.

> >  \fImeter\fR is omitted, or if it is specified as \fBall\fR, then all
> >  meters are deleted.
> >  .
> > -.IP "\fBdump\-meters \fIswitch\fR [\fImeter\fR]"
> > +.IP "[\fB\-\-oneline\fR] \fBdump\-meters \fIswitch\fR [\fImeter\fR]"
> >  Print entries from \fIswitch\fR's meter table.  To print only a
> > -specific meter, specify its number as \fImeter\fR.  Otherwise, if
> > +specific meter, specify a meter syntax \fBmeter=\fIid\fR.  Otherwise, if
>
> ... and here?
>
> >  \fImeter\fR is omitted, or if it is specified as \fBall\fR, then all
> > -meters are printed.
> > +meters are printed.  With \fB\-\-oneline\fR, band information will be
> > +combined into one line.
> >  .
> >  .IP "\fBmeter\-stats \fIswitch\fR [\fImeter\fR]"
> >  Print meter statistics.  \fImeter\fR can specify a single meter with
> > @@ -1342,6 +1354,11 @@ includes flow duration, packet and byte counts, and idle and hard age
> >  in its output.  With \fB\-\-no\-stats\fR, it omits all of these, as
> >  well as cookie values and table IDs if they are zero.
> >  .
> > +.IP "\fB\-\-oneline\fR"
> > +The \fBdump\-meters\fR command prints each band in a separate line by
> > +default. With \fB\-\-oneline\fR, all bands will be printed in a single
> > +line. This is useful for dumping meters to a file and restoring them.
> > +.
> >  .IP "\fB\-\-read-only\fR"
> >  Do not execute read/write commands.
> >  .
> > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> > index eabec18a3..b9578d682 100644
> > --- a/utilities/ovs-ofctl.c
> > +++ b/utilities/ovs-ofctl.c
> > @@ -157,6 +157,9 @@ static int print_pcap = 0;
> >  /* --raw: Makes "ofp-print" read binary data from stdin. */
> >  static int raw = 0;
> >
> > +/* --oneline: show meter config in a single line. */
> > +static int oneline = 0;
> > +
>
> nit: bool would be a better type for 'online'.
>      Likewise, there could be a cleanup patch for similar variables
>      that are currently of type int.
>
OK

> >  static const struct ovs_cmdl_command *get_all_commands(void);
> >
> >  OVS_NO_RETURN static void usage(void);
> > @@ -245,6 +248,7 @@ parse_options(int argc, char *argv[])
> >          {"pcap", no_argument, &print_pcap, 1},
> >          {"raw", no_argument, &raw, 1},
> >          {"read-only", no_argument, NULL, OPT_READ_ONLY},
> > +        {"oneline", no_argument, &oneline, 1},
> >          DAEMON_LONG_OPTIONS,
> >          OFP_VERSION_LONG_OPTIONS,
> >          VLOG_LONG_OPTIONS,
> > @@ -475,9 +479,10 @@ usage(void)
> >             "  dump-group-stats SWITCH [GROUP]  print group statistics\n"
> >             "  queue-get-config SWITCH [PORT]  print queue config for PORT\n"
> >             "  add-meter SWITCH METER      add meter described by METER\n"
> > +           "  add-meters SWITCH FILE      add meters from FILE\n"
> >             "  mod-meter SWITCH METER      modify specific METER\n"
> >             "  del-meters SWITCH [METER]   delete meters matching METER\n"
> > -           "  dump-meters SWITCH [METER]  print METER configuration\n"
> > +           "  dump-meters SWITCH [METER]  print METER entries\n"
> >             "  meter-stats SWITCH [METER]  print meter statistics\n"
> >             "  meter-features SWITCH       print meter features\n"
> >             "  add-tlv-map SWITCH MAP      add TLV option MAPpings\n"
> > @@ -515,6 +520,7 @@ usage(void)
> >             "  --rsort[=field]             sort in descending order\n"
> >             "  --names                     show port names instead of numbers\n"
> >             "  --no-names                  show port numbers, but not names\n"
> > +           "  --oneline                   show meter bands in a single line\n"
> >             "  --unixctl=SOCKET            set control socket name\n"
> >             "  --color[=always|never|auto] control use of color in output\n"
> >             "  -h, --help                  display this help message\n"
> > @@ -1443,7 +1449,7 @@ set_protocol_for_flow_dump(struct vconn *vconn,
> >      if (usable_protocols & allowed_protocols) {
> >          ovs_fatal(0, "switch does not support any of the usable flow "
> >                    "formats (%s)", usable_s);
> > -} else {
> > +    } else {
> >          char *allowed_s = ofputil_protocols_to_string(allowed_protocols);
> >          ovs_fatal(0, "none of the usable flow formats (%s) is among the "
> >                    "allowed flow formats (%s)", usable_s, allowed_s);
> > @@ -4080,57 +4086,107 @@ ofctl_diff_flows(struct ovs_cmdl_context *ctx)
> >  }
> >
> >  static void
> > -ofctl_meter_mod__(const char *bridge, const char *str, int command)
> > +ofctl_meter_mod__(const char *remote, struct ofputil_meter_mod *mms,
> > +                  size_t n_mms, enum ofputil_protocol usable_protocols)
> >  {
> > -    struct ofputil_meter_mod mm;
> > +    struct ofputil_meter_mod *mm;
> >      struct vconn *vconn;
> >      enum ofputil_protocol protocol;
> > -    enum ofputil_protocol usable_protocols;
> >      enum ofp_version version;
> > +    struct ofpbuf *request;
> > +    size_t i;
> >
> > -    memset(&mm, 0, sizeof mm);
> > -    if (str) {
> > -        char *error;
> > -        error = parse_ofp_meter_mod_str(&mm, str, command, &usable_protocols);
> > -        if (error) {
> > -            ovs_fatal(0, "%s", error);
> > -        }
> > -    } else {
> > -        usable_protocols = OFPUTIL_P_OF13_UP;
> > -        mm.command = command;
> > -        mm.meter.meter_id = OFPM13_ALL;
> > +    protocol = open_vconn_for_flow_mod(remote, &vconn, usable_protocols);
> > +    version = ofputil_protocol_to_ofp_version(protocol);
> > +
> > +     for (i = 0; i < n_mms; i++) {
> > +        mm = &mms[i];
> > +        request = ofputil_encode_meter_mod(version, mm);
> > +        transact_noreply(vconn, request);
> > +        free(mm->meter.bands);
> >      }
> >
> > -    protocol = open_vconn_for_flow_mod(bridge, &vconn, usable_protocols);
> > -    version = ofputil_protocol_to_ofp_version(protocol);
> > -    transact_noreply(vconn, ofputil_encode_meter_mod(version, &mm));
> > -    free(mm.meter.bands);
> >      vconn_close(vconn);
> >  }
> >
> >  static void
> > -ofctl_meter_request__(const char *bridge, const char *str,
> > -                      enum ofputil_meter_request_type type)
> > +ofctl_meter_mod_file(int argc OVS_UNUSED, char *argv[], int command)
> > +{
> > +    enum ofputil_protocol usable_protocols;
> > +    struct ofputil_meter_mod *mms = NULL;
> > +    size_t n_mms = 0;
> > +    char *error;
> > +
> > +    if (command == OFPMC13_ADD) {
> > +        /* Allow the file to specify a mix of commands. If none specified at
> > +         * the beginning of any given line, then the default is OFPMC13_ADD, so
> > +         * this is backwards compatible. */
> > +        command = -2;
> > +    }
> > +    error = parse_ofp_meter_mod_file(argv[2], command,
> > +                                     &mms, &n_mms, &usable_protocols);
> > +    if (error) {
> > +        ovs_fatal(0, "%s", error);
> > +    }
> > +    ofctl_meter_mod__(argv[1], mms, n_mms, usable_protocols);
> > +    free(mms);
> > +}
> > +
> > +static void
> > +ofctl_meter_mod(int argc, char *argv[], uint16_t command)
> > +{
> > +    if (argc > 2 && !strcmp(argv[2], "-")) {
> > +        ofctl_meter_mod_file(argc, argv, command);
> > +    } else {
> > +        enum ofputil_protocol usable_protocols;
> > +        struct ofputil_meter_mod mm;
> > +        char *error;
> > +        memset(&mm, 0, sizeof mm);
> > +        error = parse_ofp_meter_mod_str(&mm, argc > 2 ? argv[2] : "", command,
> > +                                        &usable_protocols);
> > +        if (error) {
> > +            ovs_fatal(0, "%s", error);
> > +        }
> > +        ofctl_meter_mod__(argv[1], &mm, 1, usable_protocols);
> > +    }
> > +}
>
> nit: blank line
>
OK

> > +static struct vconn *
> > +prepare_dump_meters(const char *bridge, const char *str,
> > +                    struct ofputil_meter_mod *mm,
> > +                    enum ofputil_protocol *protocolp)
> >  {
> > -    struct ofputil_meter_mod mm;
> >      struct vconn *vconn;
> >      enum ofputil_protocol usable_protocols;
> >      enum ofputil_protocol protocol;
> > -    enum ofp_version version;
> >
> > -    memset(&mm, 0, sizeof mm);
> >      if (str) {
> >          char *error;
> > -        error = parse_ofp_meter_mod_str(&mm, str, -1, &usable_protocols);
> > +        error = parse_ofp_meter_mod_str(mm, str, -1, &usable_protocols);
> >          if (error) {
> >              ovs_fatal(0, "%s", error);
> >          }
> >      } else {
> >          usable_protocols = OFPUTIL_P_OF13_UP;
> > -        mm.meter.meter_id = OFPM13_ALL;
> > +        mm->meter.meter_id = OFPM13_ALL;
> >      }
> >
> > -    protocol = open_vconn_for_flow_mod(bridge, &vconn, usable_protocols);
> > +    protocol = open_vconn(bridge, &vconn);
> > +    *protocolp = set_protocol_for_flow_dump(vconn, protocol, usable_protocols);
> > +    return vconn;
> > +}
> > +
> > +
> > +static void
> > +ofctl_meter_request__(const char *bridge, const char *str,
> > +                      enum ofputil_meter_request_type type)
> > +{
> > +    struct ofputil_meter_mod mm;
> > +    enum ofputil_protocol protocol;
> > +    struct vconn *vconn;
> > +    enum ofp_version version;
> > +
> > +    memset(&mm, 0, sizeof mm);
> > +    vconn = prepare_dump_meters(bridge, str, &mm, &protocol);
> >      version = ofputil_protocol_to_ofp_version(protocol);
> >      dump_transaction(vconn, ofputil_encode_meter_request(version, type,
> >                                                           mm.meter.meter_id));
> > @@ -4138,32 +4194,190 @@ ofctl_meter_request__(const char *bridge, const char *str,
> >      vconn_close(vconn);
> >  }
> >
> > -
> >  static void
> >  ofctl_add_meter(struct ovs_cmdl_context *ctx)
> >  {
> > -    ofctl_meter_mod__(ctx->argv[1], ctx->argv[2], OFPMC13_ADD);
> > +    ofctl_meter_mod(ctx->argc, ctx->argv, OFPMC13_ADD);
> > +}
> > +
> > +static void
> > +ofctl_add_meters(struct ovs_cmdl_context *ctx)
> > +{
> > +    ofctl_meter_mod_file(ctx->argc, ctx->argv, OFPMC13_ADD);
> >  }
> >
> >  static void
> >  ofctl_mod_meter(struct ovs_cmdl_context *ctx)
> >  {
> > -    ofctl_meter_mod__(ctx->argv[1], ctx->argv[2], OFPMC13_MODIFY);
> > +    ofctl_meter_mod(ctx->argc, ctx->argv, OFPMC13_MODIFY);
> >  }
> >
> >  static void
> >  ofctl_del_meters(struct ovs_cmdl_context *ctx)
> >  {
> > -    ofctl_meter_mod__(ctx->argv[1], ctx->argc > 2 ? ctx->argv[2] : NULL, OFPMC13_DELETE);
> > +    ofctl_meter_mod(ctx->argc, ctx->argv, OFPMC13_DELETE);
> >  }
> >
> >  static void
> > -ofctl_dump_meters(struct ovs_cmdl_context *ctx)
> > +ofctl_dump_meters__(struct ovs_cmdl_context *ctx)
> >  {
> >      ofctl_meter_request__(ctx->argv[1], ctx->argc > 2 ? ctx->argv[2] : NULL,
> >                            OFPUTIL_METER_CONFIG);
> >  }
> >
> > +static int
> > +recv_meter_stats_reply(struct vconn *vconn, ovs_be32 send_xid,
> > +                       struct ofpbuf **replyp,
> > +                       struct ofputil_meter_config *mc, struct ofpbuf *bands)
> > +{
> > +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> > +    struct ofpbuf *reply = *replyp;
> > +
> > +    for (;;) {
> > +        int retval;
> > +        bool more;
> > +
> > +        if (!reply) {
> > +            enum ofptype type;
> > +            enum ofperr error;
> > +
> > +            do {
> > +                error = vconn_recv_block(vconn, &reply);
> > +                if (error) {
> > +                    return error;
> > +                }
> > +            } while (((struct ofp_header *) reply->data)->xid != send_xid);
> > +
> > +            error = ofptype_decode(&type, reply->data);
> > +            if (error || type != OFPTYPE_METER_CONFIG_STATS_REPLY) {
> > +                VLOG_WARN_RL(&rl, "received bad reply: %s",
> > +                             ofp_to_string(reply->data, reply->size,
> > +                                           NULL, NULL, 1));
> > +                return EPROTO;
> > +            }
> > +        }
> > +
> > +        /* Pull an individual meter reply out of the message. */
> > +        retval = ofputil_decode_meter_config(reply, mc, bands);
> > +        switch (retval) {
> > +        case 0:
> > +            *replyp = reply;
> > +            return 0;
> > +
> > +        case EOF:
> > +            more = ofpmp_more(reply->header);
> > +            ofpbuf_delete(reply);
> > +            reply = NULL;
> > +            if (!more) {
> > +                *replyp = NULL;
> > +                return EOF;
> > +            }
> > +            break;
> > +
> > +        default:
> > +            VLOG_WARN_RL(&rl, "parse error in reply (%s)",
> > +                         ofperr_to_string(retval));
> > +            return EPROTO;
> > +        }
> > +    }
> > +}
> > +
> > +static int
> > +vconn_dump_meters(struct vconn *vconn,
> > +                 struct ofpbuf *request,
> > +                 struct ofputil_meter_config **mcsp, size_t *n_mcsp)
> > +{
> > +    struct ofputil_meter_config *mcs = NULL;
> > +    size_t n_mcs = 0;
> > +    size_t allocated_mcs = 0;
> > +
> > +    const struct ofp_header *oh = request->data;
> > +    ovs_be32 send_xid = oh->xid;
> > +    int error = vconn_send_block(vconn, request);
> > +    if (error) {
> > +        goto exit;
> > +    }
> > +
> > +    struct ofpbuf *reply = NULL;
> > +    struct ofpbuf bands;
> > +    ofpbuf_init(&bands, 64);
> > +    for (;;) {
> > +        if (n_mcs >= allocated_mcs) {
> > +            mcs = x2nrealloc(mcs, &allocated_mcs, sizeof *mcs);
> > +        }
> > +
> > +        struct ofputil_meter_config *mc = &mcs[n_mcs];
> > +        error = recv_meter_stats_reply(vconn, send_xid, &reply, mc, &bands);
> > +        if (error) {
> > +            if (error == EOF) {
> > +                error = 0;
> > +            }
> > +            break;
> > +        }
> > +        mc->bands = xmemdup(mc->bands, mc->n_bands * sizeof(mc->bands[0]));
> > +        n_mcs++;
> > +    }
> > +    ofpbuf_uninit(&bands);
> > +    ofpbuf_delete(reply);
> > +
> > +    if (error) {
> > +        for (size_t i = 0; i < n_mcs; i++) {
> > +            free(CONST_CAST(struct ofputil_meter_band *, mcs[i].bands));
> > +        }
> > +        free(mcs);
> > +
> > +        mcs = NULL;
> > +        n_mcs = 0;
> > +    }
> > +
> > +exit:
> > +    *mcsp = mcs;
> > +    *n_mcsp = n_mcs;
> > +    return error;
> > +}
> > +
> > +static void
> > +ofctl_dump_meters(struct ovs_cmdl_context *ctx)
> > +{
> > +    if (!oneline) {
> > +        ofctl_dump_meters__(ctx);
> > +    } else {
> > +        struct ofputil_meter_mod mm;
> > +        struct vconn *vconn;
> > +        enum ofputil_protocol protocol;
> > +        enum ofp_version version;
> > +
> > +        memset(&mm, 0, sizeof mm);
> > +        const char *bridge = ctx->argv[1];
> > +        const char *str = ctx->argc > 2 ? ctx->argv[2] : NULL;
> > +
> > +        vconn = prepare_dump_meters(bridge, str, &mm, &protocol);
> > +        version = ofputil_protocol_to_ofp_version(protocol);
> > +        struct ofpbuf *request = ofputil_encode_meter_request(version,
> > +                        OFPUTIL_METER_CONFIG, mm.meter.meter_id);
>
> The logic in the two 'paragraphs' above seems to largely duplicate
> ofctl_dump_meters__() => ofctl_meter_request__().
>
> Could those functions be parameterised over oneline?
>
ofctl_meter_request__() will handle stats, features requests. We can
put 'oneline' to
parameters, but that will make other functions a little confused about
this parameter.
A combined function will be a little more complicated since it will
have to judge the
request type, changes won't save us any code.

> > +
> > +        struct ofputil_meter_config *mcs;
> > +        size_t n_mtrs;
> > +        run(vconn_dump_meters(vconn, request, &mcs, &n_mtrs),
> > +            "dump meters");
> > +
> > +        struct ds s = DS_EMPTY_INITIALIZER;
> > +        for (size_t i = 0; i < n_mtrs; i ++) {
> > +            ds_clear(&s);
> > +            ofputil_format_meter_config(&s, &mcs[i], 1);
> > +            printf("%s\n", ds_cstr(&s));
> > +        }
> > +        ds_destroy(&s);
> > +
> > +        for (size_t i = 0; i < n_mtrs; i ++) {
> > +            free(CONST_CAST(struct ofputil_meter_band *, mcs[i].bands));
> > +        }
> > +        free(mcs);
>
> The above, ofputil_format_meter_config(), and recv_meter_stats_reply()
> seems to be a lot of code to customise what was otherwise a call to
> dump_transaction().
>
> Perhaps it would be cleaner to parameterise ofp_print() over 'oneline'.
>
That was my first thought, then I realize that I would have to add
this parameter to several
functions recursively, like dump_transaction(), ofp_print(),
ofp_to_string__(), ofp_to_string().
I don't think these functions really need that parameter since only
meter has compatibility issues.

Regards,
Wan Junjie

>
> > +        free(mm.meter.bands);
> > +        vconn_close(vconn);
> > +    }
> > +}
> > +
> >  static void
> >  ofctl_meter_stats(struct ovs_cmdl_context *ctx)
> >  {
> > @@ -5068,15 +5282,17 @@ static const struct ovs_cmdl_command all_commands[] = {
> >        2, 2, ofctl_diff_flows, OVS_RW },
> >      { "add-meter", "switch meter",
> >        2, 2, ofctl_add_meter, OVS_RW },
> > +    { "add-meters", "switch file",
> > +      2, 2, ofctl_add_meters, OVS_RW },
> >      { "mod-meter", "switch meter",
> >        2, 2, ofctl_mod_meter, OVS_RW },
> > -    { "del-meter", "switch meter",
> > +    { "del-meter", "switch [meter]",
> >        1, 2, ofctl_del_meters, OVS_RW },
> >      { "del-meters", "switch",
> >        1, 2, ofctl_del_meters, OVS_RW },
> >      { "dump-meter", "switch meter",
> >        1, 2, ofctl_dump_meters, OVS_RO },
> > -    { "dump-meters", "switch",
> > +    { "dump-meters", "switch [meter]",
> >        1, 2, ofctl_dump_meters, OVS_RO },
> >      { "meter-stats", "switch [meter]",
> >        1, 2, ofctl_meter_stats, OVS_RO },
> > diff --git a/utilities/ovs-save b/utilities/ovs-save
> > index 67092ecf7..d1baa3415 100755
> > --- a/utilities/ovs-save
> > +++ b/utilities/ovs-save
> > @@ -139,6 +139,9 @@ save_flows () {
> >          echo "ovs-ofctl -O $ofp_version add-groups ${bridge} \
> >                \"$workdir/$bridge.groups.dump\" ${bundle}"
> >
> > +        echo "ovs-ofctl -O $ofp_version add-meters ${bridge} \
> > +              \"$workdir/$bridge.meters.dump\""
> > +
> >          echo "ovs-ofctl -O $ofp_version replace-flows ${bridge} \
> >                \"$workdir/$bridge.flows.dump\" ${bundle}"
> >
> > @@ -147,6 +150,11 @@ save_flows () {
> >                  -e '/^NXST_GROUP_DESC/d' > \
> >                  "$workdir/$bridge.groups.dump"
> >
> > +        ovs-ofctl -O $ofp_version dump-meters "$bridge" --oneline | \
> > +            sed -e '/^OFPST_METER_CONFIG/d' \
> > +                -e '/^NXST_METER_CONFIG/d' > \
> > +                "$workdir/$bridge.meters.dump"
> > +
> >          ovs-ofctl -O $ofp_version dump-flows --no-names --no-stats "$bridge" | \
> >              sed -e '/NXST_FLOW/d' \
> >                  -e '/OFPST_FLOW/d' \
> > --
> > 2.39.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >

On Sat, Mar 04, 2023 at 11:23:50AM +0800, Wan Junjie wrote:
> Hi Simon,
> 
> Thanks for the review.

Hi Wan Junjie,

Thanks for responding.

> On Fri, Mar 3, 2023 at 11:06 PM Simon Horman <simon.horman@corigine.com> wrote:
> > On Wed, Mar 01, 2023 at 04:05:17PM +0800, Wan Junjie wrote:

...

> > > diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> > > index 0a611b2ee..c44091906 100644
> > > --- a/utilities/ovs-ofctl.8.in
> > > +++ b/utilities/ovs-ofctl.8.in
> > > @@ -492,23 +492,35 @@ the \fBBridge\fR table).  For more information, see ``Q: What versions
> > >  of OpenFlow does Open vSwitch support?'' in the Open vSwitch FAQ.
> > >  .
> > >  .IP "\fBadd\-meter \fIswitch meter\fR"
> > > +.IQ "\fBadd\-meter \fIswitch - < file\fR"
> > >  Add a meter entry to \fIswitch\fR's tables. The \fImeter\fR syntax is
> > >  described in section \fBMeter Syntax\fR, below.
> > >  .
> > > +.IP "\fBadd\-meters \fIswitch file\fR"
> > > +Add each meter entry to \fIswitch\fR's tables. Each meter specification
> > > +(each line in file) may start with \fBadd\fI, \fBmodify\fI or \fBdelete\fI
> > > +keyword to specify whether a meter is to be added, modified, or deleted.
> > > +For backwards compatibility a meter specification without one of these keywords
> > > +is treated as a meter add. The \fImeter\fR syntax is described in section
> > > +\fBMeter Syntax\fR, below.
> > > +.
> > >  .IP "\fBmod\-meter \fIswitch meter\fR"
> > > +.IQ "\fBmod\-meter \fIswitch - < file\fR"
> > >  Modify an existing meter.
> > >  .
> > >  .IP "\fBdel\-meters \fIswitch\fR [\fImeter\fR]"
> > > +.IQ "\fBdel\-meters \fIswitch\fR - < file"
> > >  Delete entries from \fIswitch\fR's meter table.  To delete only a
> > > -specific meter, specify its number as \fImeter\fR.  Otherwise, if
> > > +specific meter, specify a meter syntax \fBmeter=\fIid\fR.  Otherwise, if
> >
> > Are there backwards compatibility concerns here...
> >
> In fact, old docs are not clear on meter syntax, it means meter=id.

Ok, so the documentation is being fixed to match the current implementation?
If so I think that should be a separate patch.

> > >  \fImeter\fR is omitted, or if it is specified as \fBall\fR, then all
> > >  meters are deleted.
> > >  .
> > > -.IP "\fBdump\-meters \fIswitch\fR [\fImeter\fR]"
> > > +.IP "[\fB\-\-oneline\fR] \fBdump\-meters \fIswitch\fR [\fImeter\fR]"
> > >  Print entries from \fIswitch\fR's meter table.  To print only a
> > > -specific meter, specify its number as \fImeter\fR.  Otherwise, if
> > > +specific meter, specify a meter syntax \fBmeter=\fIid\fR.  Otherwise, if
> >
> > ... and here?
> >
> > >  \fImeter\fR is omitted, or if it is specified as \fBall\fR, then all
> > > -meters are printed.
> > > +meters are printed.  With \fB\-\-oneline\fR, band information will be
> > > +combined into one line.
> > >  .
> > >  .IP "\fBmeter\-stats \fIswitch\fR [\fImeter\fR]"
> > >  Print meter statistics.  \fImeter\fR can specify a single meter with
> > > @@ -1342,6 +1354,11 @@ includes flow duration, packet and byte counts, and idle and hard age
> > >  in its output.  With \fB\-\-no\-stats\fR, it omits all of these, as
> > >  well as cookie values and table IDs if they are zero.
> > >  .
> > > +.IP "\fB\-\-oneline\fR"
> > > +The \fBdump\-meters\fR command prints each band in a separate line by
> > > +default. With \fB\-\-oneline\fR, all bands will be printed in a single
> > > +line. This is useful for dumping meters to a file and restoring them.
> > > +.
> > >  .IP "\fB\-\-read-only\fR"
> > >  Do not execute read/write commands.
> > >  .

...

> > > +static void
> > > +ofctl_dump_meters(struct ovs_cmdl_context *ctx)
> > > +{
> > > +    if (!oneline) {
> > > +        ofctl_dump_meters__(ctx);
> > > +    } else {
> > > +        struct ofputil_meter_mod mm;
> > > +        struct vconn *vconn;
> > > +        enum ofputil_protocol protocol;
> > > +        enum ofp_version version;
> > > +
> > > +        memset(&mm, 0, sizeof mm);
> > > +        const char *bridge = ctx->argv[1];
> > > +        const char *str = ctx->argc > 2 ? ctx->argv[2] : NULL;
> > > +
> > > +        vconn = prepare_dump_meters(bridge, str, &mm, &protocol);
> > > +        version = ofputil_protocol_to_ofp_version(protocol);
> > > +        struct ofpbuf *request = ofputil_encode_meter_request(version,
> > > +                        OFPUTIL_METER_CONFIG, mm.meter.meter_id);
> >
> > The logic in the two 'paragraphs' above seems to largely duplicate
> > ofctl_dump_meters__() => ofctl_meter_request__().
> >
> > Could those functions be parameterised over oneline?
> >
> ofctl_meter_request__() will handle stats, features requests. We can
> put 'oneline' to
> parameters, but that will make other functions a little confused about
> this parameter.
> A combined function will be a little more complicated since it will
> have to judge the
> request type, changes won't save us any code.
> 
> > > +
> > > +        struct ofputil_meter_config *mcs;
> > > +        size_t n_mtrs;
> > > +        run(vconn_dump_meters(vconn, request, &mcs, &n_mtrs),
> > > +            "dump meters");
> > > +
> > > +        struct ds s = DS_EMPTY_INITIALIZER;
> > > +        for (size_t i = 0; i < n_mtrs; i ++) {
> > > +            ds_clear(&s);
> > > +            ofputil_format_meter_config(&s, &mcs[i], 1);
> > > +            printf("%s\n", ds_cstr(&s));
> > > +        }
> > > +        ds_destroy(&s);
> > > +
> > > +        for (size_t i = 0; i < n_mtrs; i ++) {
> > > +            free(CONST_CAST(struct ofputil_meter_band *, mcs[i].bands));
> > > +        }
> > > +        free(mcs);
> >
> > The above, ofputil_format_meter_config(), and recv_meter_stats_reply()
> > seems to be a lot of code to customise what was otherwise a call to
> > dump_transaction().
> >
> > Perhaps it would be cleaner to parameterise ofp_print() over 'oneline'.
> >
> That was my first thought, then I realize that I would have to add
> this parameter to several
> functions recursively, like dump_transaction(), ofp_print(),
> ofp_to_string__(), ofp_to_string().
> I don't think these functions really need that parameter since only
> meter has compatibility issues.

I understand your point. But I think it would be better to teach the
core/common code how to handle this and avoid open coding the special case.

Perhaps a 'oneline' parameter isn't the best approach.
But really one bit of information is needed in order
for core/common code to implement a (slightly) different behaviour.

And it is conceivable that other dump functions will need
alternate behaviour in future.

> Regards,
> Wan Junjie

...

Hi Simon Horman,

Thanks for your review.


On Mon, Mar 6, 2023 at 11:23 PM Simon Horman <simon.horman@corigine.com> wrote:
>
> On Sat, Mar 04, 2023 at 11:23:50AM +0800, Wan Junjie wrote:
> > Hi Simon,
> >
> > Thanks for the review.
>
> Hi Wan Junjie,
>
> Thanks for responding.
>
> > On Fri, Mar 3, 2023 at 11:06 PM Simon Horman <simon.horman@corigine.com> wrote:
> > > On Wed, Mar 01, 2023 at 04:05:17PM +0800, Wan Junjie wrote:
>
> ...
>
> > > > diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> > > > index 0a611b2ee..c44091906 100644
> > > > --- a/utilities/ovs-ofctl.8.in
> > > > +++ b/utilities/ovs-ofctl.8.in
> > > > @@ -492,23 +492,35 @@ the \fBBridge\fR table).  For more information, see ``Q: What versions
> > > >  of OpenFlow does Open vSwitch support?'' in the Open vSwitch FAQ.
> > > >  .
> > > >  .IP "\fBadd\-meter \fIswitch meter\fR"
> > > > +.IQ "\fBadd\-meter \fIswitch - < file\fR"
> > > >  Add a meter entry to \fIswitch\fR's tables. The \fImeter\fR syntax is
> > > >  described in section \fBMeter Syntax\fR, below.
> > > >  .
> > > > +.IP "\fBadd\-meters \fIswitch file\fR"
> > > > +Add each meter entry to \fIswitch\fR's tables. Each meter specification
> > > > +(each line in file) may start with \fBadd\fI, \fBmodify\fI or \fBdelete\fI
> > > > +keyword to specify whether a meter is to be added, modified, or deleted.
> > > > +For backwards compatibility a meter specification without one of these keywords
> > > > +is treated as a meter add. The \fImeter\fR syntax is described in section
> > > > +\fBMeter Syntax\fR, below.
> > > > +.
> > > >  .IP "\fBmod\-meter \fIswitch meter\fR"
> > > > +.IQ "\fBmod\-meter \fIswitch - < file\fR"
> > > >  Modify an existing meter.
> > > >  .
> > > >  .IP "\fBdel\-meters \fIswitch\fR [\fImeter\fR]"
> > > > +.IQ "\fBdel\-meters \fIswitch\fR - < file"
> > > >  Delete entries from \fIswitch\fR's meter table.  To delete only a
> > > > -specific meter, specify its number as \fImeter\fR.  Otherwise, if
> > > > +specific meter, specify a meter syntax \fBmeter=\fIid\fR.  Otherwise, if
> > >
> > > Are there backwards compatibility concerns here...
> > >
> > In fact, old docs are not clear on meter syntax, it means meter=id.
>
> Ok, so the documentation is being fixed to match the current implementation?
> If so I think that should be a separate patch.
>
Yes, and it is pointed out by Adrián Moreno in the first reply to this patch.
I will do the split in a later patch.

> > > >  \fImeter\fR is omitted, or if it is specified as \fBall\fR, then all
> > > >  meters are deleted.
> > > >  .
> > > > -.IP "\fBdump\-meters \fIswitch\fR [\fImeter\fR]"
> > > > +.IP "[\fB\-\-oneline\fR] \fBdump\-meters \fIswitch\fR [\fImeter\fR]"
> > > >  Print entries from \fIswitch\fR's meter table.  To print only a
> > > > -specific meter, specify its number as \fImeter\fR.  Otherwise, if
> > > > +specific meter, specify a meter syntax \fBmeter=\fIid\fR.  Otherwise, if
> > >
> > > ... and here?
> > >
> > > >  \fImeter\fR is omitted, or if it is specified as \fBall\fR, then all
> > > > -meters are printed.
> > > > +meters are printed.  With \fB\-\-oneline\fR, band information will be
> > > > +combined into one line.
> > > >  .
> > > >  .IP "\fBmeter\-stats \fIswitch\fR [\fImeter\fR]"
> > > >  Print meter statistics.  \fImeter\fR can specify a single meter with
> > > > @@ -1342,6 +1354,11 @@ includes flow duration, packet and byte counts, and idle and hard age
> > > >  in its output.  With \fB\-\-no\-stats\fR, it omits all of these, as
> > > >  well as cookie values and table IDs if they are zero.
> > > >  .
> > > > +.IP "\fB\-\-oneline\fR"
> > > > +The \fBdump\-meters\fR command prints each band in a separate line by
> > > > +default. With \fB\-\-oneline\fR, all bands will be printed in a single
> > > > +line. This is useful for dumping meters to a file and restoring them.
> > > > +.
> > > >  .IP "\fB\-\-read-only\fR"
> > > >  Do not execute read/write commands.
> > > >  .
>
> ...
>
> > > > +static void
> > > > +ofctl_dump_meters(struct ovs_cmdl_context *ctx)
> > > > +{
> > > > +    if (!oneline) {
> > > > +        ofctl_dump_meters__(ctx);
> > > > +    } else {
> > > > +        struct ofputil_meter_mod mm;
> > > > +        struct vconn *vconn;
> > > > +        enum ofputil_protocol protocol;
> > > > +        enum ofp_version version;
> > > > +
> > > > +        memset(&mm, 0, sizeof mm);
> > > > +        const char *bridge = ctx->argv[1];
> > > > +        const char *str = ctx->argc > 2 ? ctx->argv[2] : NULL;
> > > > +
> > > > +        vconn = prepare_dump_meters(bridge, str, &mm, &protocol);
> > > > +        version = ofputil_protocol_to_ofp_version(protocol);
> > > > +        struct ofpbuf *request = ofputil_encode_meter_request(version,
> > > > +                        OFPUTIL_METER_CONFIG, mm.meter.meter_id);
> > >
> > > The logic in the two 'paragraphs' above seems to largely duplicate
> > > ofctl_dump_meters__() => ofctl_meter_request__().
> > >
> > > Could those functions be parameterised over oneline?
> > >
> > ofctl_meter_request__() will handle stats, features requests. We can
> > put 'oneline' to
> > parameters, but that will make other functions a little confused about
> > this parameter.
> > A combined function will be a little more complicated since it will
> > have to judge the
> > request type, changes won't save us any code.
> >
> > > > +
> > > > +        struct ofputil_meter_config *mcs;
> > > > +        size_t n_mtrs;
> > > > +        run(vconn_dump_meters(vconn, request, &mcs, &n_mtrs),
> > > > +            "dump meters");
> > > > +
> > > > +        struct ds s = DS_EMPTY_INITIALIZER;
> > > > +        for (size_t i = 0; i < n_mtrs; i ++) {
> > > > +            ds_clear(&s);
> > > > +            ofputil_format_meter_config(&s, &mcs[i], 1);
> > > > +            printf("%s\n", ds_cstr(&s));
> > > > +        }
> > > > +        ds_destroy(&s);
> > > > +
> > > > +        for (size_t i = 0; i < n_mtrs; i ++) {
> > > > +            free(CONST_CAST(struct ofputil_meter_band *, mcs[i].bands));
> > > > +        }
> > > > +        free(mcs);
> > >
> > > The above, ofputil_format_meter_config(), and recv_meter_stats_reply()
> > > seems to be a lot of code to customise what was otherwise a call to
> > > dump_transaction().
> > >
> > > Perhaps it would be cleaner to parameterise ofp_print() over 'oneline'.
> > >
> > That was my first thought, then I realize that I would have to add
> > this parameter to several
> > functions recursively, like dump_transaction(), ofp_print(),
> > ofp_to_string__(), ofp_to_string().
> > I don't think these functions really need that parameter since only
> > meter has compatibility issues.
>
> I understand your point. But I think it would be better to teach the
> core/common code how to handle this and avoid open coding the special case.
>
> Perhaps a 'oneline' parameter isn't the best approach.
> But really one bit of information is needed in order
> for core/common code to implement a (slightly) different behaviour.
>
> And it is conceivable that other dump functions will need
> alternate behaviour in future.
>
OK, I will see if I can merge it to 'verbosity'.



> > Regards,
> > Wan Junjie
>
> ...

On Tue, Mar 07, 2023 at 12:24:55PM +0800, Wan Junjie wrote:
> Hi Simon Horman,
> 
> Thanks for your review.
> 
> 
> On Mon, Mar 6, 2023 at 11:23 PM Simon Horman <simon.horman@corigine.com> wrote:
> > On Sat, Mar 04, 2023 at 11:23:50AM +0800, Wan Junjie wrote:
...
> > > On Fri, Mar 3, 2023 at 11:06 PM Simon Horman <simon.horman@corigine.com> wrote:
> > > > On Wed, Mar 01, 2023 at 04:05:17PM +0800, Wan Junjie wrote:

...

> > > > > +        struct ofputil_meter_config *mcs;
> > > > > +        size_t n_mtrs;
> > > > > +        run(vconn_dump_meters(vconn, request, &mcs, &n_mtrs),
> > > > > +            "dump meters");
> > > > > +
> > > > > +        struct ds s = DS_EMPTY_INITIALIZER;
> > > > > +        for (size_t i = 0; i < n_mtrs; i ++) {
> > > > > +            ds_clear(&s);
> > > > > +            ofputil_format_meter_config(&s, &mcs[i], 1);
> > > > > +            printf("%s\n", ds_cstr(&s));
> > > > > +        }
> > > > > +        ds_destroy(&s);
> > > > > +
> > > > > +        for (size_t i = 0; i < n_mtrs; i ++) {
> > > > > +            free(CONST_CAST(struct ofputil_meter_band *, mcs[i].bands));
> > > > > +        }
> > > > > +        free(mcs);
> > > >
> > > > The above, ofputil_format_meter_config(), and recv_meter_stats_reply()
> > > > seems to be a lot of code to customise what was otherwise a call to
> > > > dump_transaction().
> > > >
> > > > Perhaps it would be cleaner to parameterise ofp_print() over 'oneline'.
> > > >
> > > That was my first thought, then I realize that I would have to add
> > > this parameter to several
> > > functions recursively, like dump_transaction(), ofp_print(),
> > > ofp_to_string__(), ofp_to_string().
> > > I don't think these functions really need that parameter since only
> > > meter has compatibility issues.
> >
> > I understand your point. But I think it would be better to teach the
> > core/common code how to handle this and avoid open coding the special case.
> >
> > Perhaps a 'oneline' parameter isn't the best approach.
> > But really one bit of information is needed in order
> > for core/common code to implement a (slightly) different behaviour.
> >
> > And it is conceivable that other dump functions will need
> > alternate behaviour in future.
> >
> OK, I will see if I can merge it to 'verbosity'.

Thanks. I agree that overloading/reusing the verbosity parameter
is worth a shot.