Hi Wan,

Sorry for the delay in the review.
I've started looking at this patch. Please see some initial comments below. I'll continue on Monday.

Thanks.

On 11/15/22 05:16, 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 <wanjun...@bytedance.com>
---
v3:
add '--oneline' option for dump-meter(s) command

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

diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h
index 6776eae87..0514b4ec4 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);
struct ofputil_meter_mod {
      uint16_t command;
@@ -79,6 +80,12 @@ 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;
+

Please align all the arguments one space after the "(". Also I think "int command" fits in the first line.


  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");
          ofputil_format_meter_band(s, mc->flags, &mc->bands[i]);
      }
-    ds_put_char(s, '\n');
+    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);
+}
+
+/* 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)
+{

I would really like not to duplicate (actually triplicate) this function. Do you see an easy way to reuse one of the other almost identical functions in ofp-group.c and ofp-flow.c?

+    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));
+    }
+

This option of using stdin is not properly documented in ovs-ofctl(8). Please document it.

+    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,
+                                         &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 bd37fa17a..6d7fc857e 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -378,8 +378,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);
      }
      ofpbuf_uninit(&bands);
@@ -1269,7 +1270,7 @@ ofp_to_string(const void *oh_, size_t len,
      ds_put_hex_dump(&string, oh, len, 0, true);
      return ds_steal_cstr(&string);
  }
-
+

Why remove the section delimiter?

  static void
  print_and_free(FILE *stream, char *string)
  {
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index 6aff1eda7..2139b64c0 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
+])
+
  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 10a6a64de..ed3444fe3 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -464,20 +464,29 @@ of OpenFlow does Open vSwitch support?'' in the Open 
vSwitch FAQ.
  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 spicify whether a meter is to be added, modified, or deleted.

s/spicify/specify/

+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"
  Modify an existing meter.
  .
  .IP "\fBdel\-meters \fIswitch\fR [\fImeter\fR]"
  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
  \fImeter\fR is omitted, or if it is specified as \fBall\fR, then all
  meters are deleted.
  .
  .IP "\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
  \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
@@ -1311,6 +1320,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 by default print each band of a meter
+in a separate line. With \fB\-\-oneline\fR, all bands will be printed
+in a single line. This is useful for dump meters to a file and restore.
+.

This reads a bit strange to me (I'm not native English speaker). I'd make the following changes: s/command by default print each band of meter in separate line/command prints each band in a separate line by default/ s/This is useful for dump meters to a file and restore/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 fe9114580..37cd541b6 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -156,6 +156,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;
+
  static const struct ovs_cmdl_command *get_all_commands(void);
OVS_NO_RETURN static void usage(void);
@@ -244,6 +247,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,
@@ -474,9 +478,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] [--oneline]  print METER entries\n"

Suggesting this syntax in the "--help" and ovs-ofctl(8) seems to contract the general command syntax which is:
   ovs-ofctl [options] command [switch] [args...]

For the sake of consistency across all the commands, I would either omit the option in the short command help or at least put it in the right order.

For the documentation (ovs-ofctl(8)), I would also put it before (similar to how group commands are documented).

Also, just realized "dump-meter" (singular) is not documented here.

             "  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"
@@ -511,6 +516,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"
@@ -4032,32 +4038,70 @@ 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);

This call to memset has no point now. It's being called on a pointer.

Now that the allocation of the "struct ofputil_meter_mod" has been moved out of this function, so should the memset. In particular to ofctl_meter_mod_file() and ofctl_meter_mod().


-    if (str) {
+    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);
+    }
+
+    vconn_close(vconn);
+}
+
+static void
+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;
+

Extra space.

+    }
+    error = parse_ofp_meter_mod_file(argv[2], command,
+                                    &mms, &n_mms, &usable_protocols);

nit: Please an extra space so arguments are allinged.

+    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;
-        error = parse_ofp_meter_mod_str(&mm, str, command, &usable_protocols);
+        error = parse_ofp_meter_mod_str(&mm, argc > 2 ? argv[2] : "", 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;
+        ofctl_meter_mod__(argv[1], &mm, 1, usable_protocols);
      }
-
-    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
@@ -4090,32 +4134,201 @@ 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)
+{

I'll continue next week but my first feeling is that we should try to refactor recv_flow_stats_reply and vconn_dump_flows in lib/vconn.c so that we don't have to duplicate all this code again. Do you have any ideas?

+    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 usable_protocols;
+        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;
+        if (str) {
+            char *error;
+            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;
+        }
+
+        protocol = open_vconn_for_flow_mod(bridge, &vconn, usable_protocols);
+        version = ofputil_protocol_to_ofp_version(protocol);
+        struct ofpbuf *request = ofputil_encode_meter_request(version,
+                        OFPUTIL_METER_CONFIG, mm.meter.meter_id);
+

Duplicated code. How about we have a prepare_dump_meters that initializes the vconn for both this function and ofctl_meter_request__()?


+        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);
+        free(mm.meter.bands);
+        vconn_close(vconn);
+    }
+}
+
  static void
  ofctl_meter_stats(struct ovs_cmdl_context *ctx)
  {
@@ -5020,15 +5233,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",
+    { "dump-meter", "switch meter [--oneline]",
        1, 2, ofctl_dump_meters, OVS_RO },
-    { "dump-meters", "switch",
+    { "dump-meters", "switch [meter] [--oneline]",
        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' \


--
Adrián Moreno

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to