On 2/28/22 12:40, 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 <[email protected]>
---
v2:
fix failed testcases, as dump-meters format changes
---
  include/openvswitch/ofp-meter.h |  6 +++
  lib/ofp-meter.c                 | 94 ++++++++++++++++++++++++++++++++-
  tests/dpif-netdev.at            |  6 +--
  tests/ofp-print.at              | 13 ++---
  tests/ofproto-dpif.at           |  3 +-
  tests/ofproto.at                |  3 +-
  utilities/ovs-ofctl.c           | 86 +++++++++++++++++++++++-------
  utilities/ovs-save              |  8 +++
  8 files changed, 182 insertions(+), 37 deletions(-)

diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h
index 6776eae87..34ecbfdb2 100644
--- a/include/openvswitch/ofp-meter.h
+++ b/include/openvswitch/ofp-meter.h
@@ -79,6 +79,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;
+
  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..5fb780ba3 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");
@@ -578,6 +579,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 +625,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) {
@@ -807,3 +831,71 @@ ofputil_format_meter_mod(struct ds *s, const struct 
ofputil_meter_mod *mm)
ofputil_format_meter_config(s, &mm->meter);
  }
+
+/* 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,
+                                         &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/tests/dpif-netdev.at b/tests/dpif-netdev.at
index a79ebdb61..98c474d20 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -291,11 +291,9 @@ ovs-appctl time/stop
AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl
  OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
-meter=1 pktps burst stats bands=
-type=drop rate=1 burst_size=1
+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
+meter=2 kbps burst stats bands= type=drop rate=1 burst_size=2
  ])
ovs-appctl time/warp 5000
diff --git a/tests/ofp-print.at b/tests/ofp-print.at
index 2c7e163bd..36ebccefc 100644
--- a/tests/ofp-print.at
+++ b/tests/ofp-print.at
@@ -2371,8 +2371,7 @@ AT_CHECK([ovs-ofctl ofp-print "\
  04 1d 00 20 00 00 00 02 00 00 00 0d 00 00 00 05 \
  00 01 00 10 00 00 04 00 00 00 00 80 00 00 00 00 \
  "], [0], [dnl
-OFPT_METER_MOD (OF1.3) (xid=0x2): ADD meter=5 kbps burst stats bands=
-type=drop rate=1024 burst_size=128
+OFPT_METER_MOD (OF1.3) (xid=0x2): ADD meter=5 kbps burst stats bands= 
type=drop rate=1024 burst_size=128
  ])
  AT_CLEANUP
@@ -2455,12 +2454,9 @@ AT_CHECK([ovs-ofctl ofp-print "\
  00 01 00 10 00 02 00 00 00 00 00 00 00 00 00 00 \
  "], [0], [dnl
  OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
-meter=1 kbps burst bands=
-type=drop rate=65536 burst_size=1280
-type=dscp_remark rate=1048576 burst_size=61440 prec_level=0
+meter=1 kbps burst bands= type=drop rate=65536 burst_size=1280 
type=dscp_remark rate=1048576 burst_size=61440 prec_level=0
-meter=2 kbps stats bands=
-type=drop rate=131072
+meter=2 kbps stats bands= type=drop rate=131072
  ])
  AT_CLEANUP
@@ -3071,8 +3067,7 @@ dnl inner msg copied and pasted from the valid OFPT_METER_MOD OF1.3 test:
  04 1d 00 20 00 00 00 02 00 00 00 0d 00 00 00 05 \
  00 01 00 10 00 00 04 00 00 00 00 80 00 00 00 00 \
  "], [0], [dnl
-ONFT_REQUESTFORWARD (OF1.3) (xid=0x2): reason=meter_mod ADD meter=5 kbps burst 
stats bands=
-type=drop rate=1024 burst_size=128
+ONFT_REQUESTFORWARD (OF1.3) (xid=0x2): reason=meter_mod ADD meter=5 kbps burst 
stats bands= type=drop rate=1024 burst_size=128
  ])
  AT_CLEANUP
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 912f3a1da..52cb49774 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -9834,8 +9834,7 @@ ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 
packet=50540000000a50540000000
  # Check that vswitchd hasn't crashed by dumping the meter added above
  AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 | ofctl_strip], [0], [dnl
  OFPST_METER_CONFIG reply (OF1.3):
-meter=1 pktps bands=
-type=drop rate=1
+meter=1 pktps bands= type=drop rate=1
  ])
OVS_VSWITCHD_STOP
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 156d3e058..a6a61d24c 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -6412,8 +6412,7 @@ OFPST_GROUP_DESC reply (OF1.1) (xid=0x2):
  ])
      AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl
  OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
-meter=1 pktps burst stats bands=
-type=drop rate=1 burst_size=1
+meter=1 pktps burst stats bands= type=drop rate=1 burst_size=1
  ])
  }
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index ede7f1e61..152836276 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -474,6 +474,7 @@ 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"
@@ -4012,32 +4013,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);
-    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;
+
+    }
+    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;
-        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
@@ -4074,19 +4113,26 @@ ofctl_meter_request__(const char *bridge, const char 
*str,
  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
@@ -5000,9 +5046,11 @@ 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 },
diff --git a/utilities/ovs-save b/utilities/ovs-save
index fb2025b76..e3551b9f9 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" | \
+            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' \




Hi Wan,

Thank you for working on this.

I see that your implementation basically mimics the one in ofp-flow.c and, alghough I think it's good to have a more homogeneus user experience I don't fully understand why we'd need that much complexity.

Why do we need to be able to specify "modify" and "delete" actions within the "add-meter" and "add-meters" commands? AFAIU, those options can be helpful for unit tests in which we want to replay a potentially complex sequence of controller flow commands. I don't understand why we need that in the meter table, it being so simple and I don't see any use for it in this patch either.

On the other hand, although I agree the multi-line meter representation is not ideal, I'm not 100% sure we should simply modify it as other users might be parsing that output. The output of other ofctl commands has been considered a pseudo-API at times. Maybe some kind of "--single-line" knob can do the work. I'd like Ilya's thoughts on this

Finally, from this patch I miss:
- Documentation: There is no modification of the man page which should now need to include all the new command options. In fact, the current documentation is quite confusing because it says:

del-meters switch [meter]

    Delete entries from switch's meter table.  To delete only a specific meter, 
specify its number as meter.  Otherwise, if meter is omitted, or if it is 
specified as all, then all meters are deleted.

...which is not true: [meter] cannot be just the meter id, it has to be "meter={meter_id}".

- Unit tests for all new ways of modifying the meter table.


Thanks

--
Adrián Moreno

--
Adrián Moreno

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to