diff mbox series

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

Message ID 20230411030346.89023-1-wanjunjie@bytedance.com
State Superseded
Headers show
Series [ovs-dev,v9] utilities/ofctl: add-meters for save and restore | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Wan Junjie April 11, 2023, 3:03 a.m. UTC
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>

---
v9:
fix verbosity mask bits for testcase
apologies for mess

v8:
fix missing code for testcase

v7:
typo in code

v6:
code style

v5:
merge oneline to verbosity higher bits
remove duplicate dump_meters code

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 |   9 ++-
 include/openvswitch/ofp-print.h |  10 +++
 lib/ofp-meter.c                 | 100 ++++++++++++++++++++++-
 lib/ofp-print.c                 |  19 +++--
 tests/dpif-netdev.at            |  44 ++++++++--
 utilities/ovs-ofctl.8.in        |  25 +++++-
 utilities/ovs-ofctl.c           | 137 ++++++++++++++++++++++++--------
 utilities/ovs-save              |   8 ++
 8 files changed, 301 insertions(+), 51 deletions(-)

Comments

0-day Robot April 11, 2023, 3:19 a.m. UTC | #1
Bleep bloop.  Greetings Wan Junjie, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
#195 FILE: lib/ofp-meter.c:871:
        error = parse_ofp_meter_mod_str(&(*mms)[*n_mms], ds_cstr(&s), command,

Lines checked: 702, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Simon Horman April 11, 2023, 11:45 a.m. UTC | #2
On Tue, Apr 11, 2023 at 11:03:46AM +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>
> 
> ---
> v9:
> fix verbosity mask bits for testcase
> apologies for mess

Hi Wan,

Please consider waiting 24h between posts of the same patch.
Slowing down can be faster sometimes :)

...

> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index 874079b84..a9cba444b 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -54,6 +54,7 @@
>  #include "openvswitch/ofp-monitor.h"
>  #include "openvswitch/ofp-msgs.h"
>  #include "openvswitch/ofp-port.h"
> +#include "openvswitch/ofp-print.h"
>  #include "openvswitch/ofp-queue.h"
>  #include "openvswitch/ofp-switch.h"
>  #include "openvswitch/ofp-table.h"
> @@ -365,12 +366,17 @@ ofp_print_meter_features_reply(struct ds *s, const struct ofp_header *oh)
>  }
>  
>  static enum ofperr
> -ofp_print_meter_config_reply(struct ds *s, const struct ofp_header *oh)
> +ofp_print_meter_config_reply(struct ds *s, const struct ofp_header *oh,
> +                             bool oneline)
>  {
>      struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length));
>      struct ofpbuf bands;
>      int retval;
>  
> +    if (oneline) {
> +        ds_put_char(s, '\n');
> +    }
> +
>      ofpbuf_init(&bands, 64);
>      for (;;) {
>          struct ofputil_meter_config mc;
> @@ -379,8 +385,10 @@ 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);
> +        if (!oneline) {
> +            ds_put_char(s, '\n');
> +        }
> +        ofputil_format_meter_config(s, &mc, oneline ? true : false);
>      }
>      ofpbuf_uninit(&bands);
>  
> @@ -1090,7 +1098,8 @@ ofp_to_string__(const struct ofp_header *oh,
>          return ofp_print_meter_stats_reply(string, oh);
>  
>      case OFPTYPE_METER_CONFIG_STATS_REPLY:
> -        return ofp_print_meter_config_reply(string, oh);
> +        return ofp_print_meter_config_reply(string, oh,
> +                                            ONELINE_GET(verbosity));

Previously the verbosity parameter of this function meant just now.
Now it means oneline | verbosity.
Above the online case is handled correctly.
But I think there are other cases in this function where
verbosity (the verbosity bits) need to be handled (using VERBOSITY()).
And possibly elsewhere in this file.

This is the risk of overloading an integer like this.
We need to be very careful.

>  
>      case OFPTYPE_METER_FEATURES_STATS_REPLY:
>          return ofp_print_meter_features_reply(string, oh);
> @@ -1278,7 +1287,7 @@ ofp_to_string(const void *oh_, size_t len,
>              ofp_print_error(&string, error);
>          }
>  
> -        if (verbosity >= 5 || error) {
> +        if (VERBOSITY(verbosity) >= 5 || error) {
>              add_newline(&string);
>              ds_put_hex_dump(&string, oh, len, 0, true);
>          }
> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> index baab60a22..cbfd02ea6 100644
> --- a/tests/dpif-netdev.at
> +++ b/tests/dpif-netdev.at
> @@ -283,11 +283,6 @@ AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
>  
>  AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps burst stats bands=type=drop rate=1 burst_size=1'])
>  AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=2 kbps burst stats bands=type=drop rate=1 burst_size=2'])
> -AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=1 action=meter:1,7'])
> -AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=7 action=meter:2,1'])
> -AT_CHECK([ovs-ofctl add-flow br1 'in_port=2 action=8'])
> -AT_CHECK([ovs-ofctl add-flow br1 'in_port=8 action=2'])
> -ovs-appctl time/stop

I'm confused about why this hunk is needed.

>  
>  AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl
>  OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> @@ -298,6 +293,45 @@ 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
> +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> +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
> +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> +meter=1 pktps burst stats bands= type=drop rate=1 burst_size=1
> +])
> +
> +AT_CHECK([ovs-ofctl del-meters -O openflow13 br0])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl
> +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> +])
> +
> +AT_DATA([meters.txt], [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
> +meter=3 kbps burst stats bands= type=drop rate=2 burst_size=3
> +])
> +
> +AT_CHECK([ovs-ofctl add-meters -O openflow13 br0 meters.txt])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 --oneline], [0], [dnl
> +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> +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=3 kbps burst stats bands= type=drop rate=2 burst_size=3
> +])
> +
> +AT_CHECK([ovs-ofctl del-meters -O openflow13 br0 meter=3])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=1 action=meter:1,7'])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=7 action=meter:2,1'])
> +AT_CHECK([ovs-ofctl add-flow br1 'in_port=2 action=8'])
> +AT_CHECK([ovs-ofctl add-flow br1 'in_port=8 action=2'])
> +ovs-appctl time/stop
> +
>  ovs-appctl time/warp 5000
>  for i in `seq 1 7`; do
>    AT_CHECK(

...

> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c

...

nit: please consider reverse xmas tree for local variables -
     longest line to shortest.

> +
> +    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,28 +4199,36 @@ 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)
>  {
> +    if (oneline) {
> +        verbosity = ONELINE_SET(verbosity);
> +    }

It feels like this should be handled in parse_options().
It's a global (to this file) value.
And the oneline bit needs to be handled generically.
So it seems logical that it should also be set in general way.

>      ofctl_meter_request__(ctx->argv[1], ctx->argc > 2 ? ctx->argv[2] : NULL,
>                            OFPUTIL_METER_CONFIG);
>  }

...

> 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' \

Maybe some tests for ovs-save would be useful at some point.
Wan Junjie April 11, 2023, 12:54 p.m. UTC | #3
On Tue, Apr 11, 2023 at 7:45 PM Simon Horman <simon.horman@corigine.com> wrote:
>
> On Tue, Apr 11, 2023 at 11:03:46AM +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>
> >
> > ---
> > v9:
> > fix verbosity mask bits for testcase
> > apologies for mess
>
> Hi Wan,
>
> Please consider waiting 24h between posts of the same patch.
> Slowing down can be faster sometimes :)
>

Hi Simon,

> ...
>
> > diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> > index 874079b84..a9cba444b 100644
> > --- a/lib/ofp-print.c
> > +++ b/lib/ofp-print.c
> > @@ -54,6 +54,7 @@
> >  #include "openvswitch/ofp-monitor.h"
> >  #include "openvswitch/ofp-msgs.h"
> >  #include "openvswitch/ofp-port.h"
> > +#include "openvswitch/ofp-print.h"
> >  #include "openvswitch/ofp-queue.h"
> >  #include "openvswitch/ofp-switch.h"
> >  #include "openvswitch/ofp-table.h"
> > @@ -365,12 +366,17 @@ ofp_print_meter_features_reply(struct ds *s, const struct ofp_header *oh)
> >  }
> >
> >  static enum ofperr
> > -ofp_print_meter_config_reply(struct ds *s, const struct ofp_header *oh)
> > +ofp_print_meter_config_reply(struct ds *s, const struct ofp_header *oh,
> > +                             bool oneline)
> >  {
> >      struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length));
> >      struct ofpbuf bands;
> >      int retval;
> >
> > +    if (oneline) {
> > +        ds_put_char(s, '\n');
> > +    }
> > +
> >      ofpbuf_init(&bands, 64);
> >      for (;;) {
> >          struct ofputil_meter_config mc;
> > @@ -379,8 +385,10 @@ 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);
> > +        if (!oneline) {
> > +            ds_put_char(s, '\n');
> > +        }
> > +        ofputil_format_meter_config(s, &mc, oneline ? true : false);
> >      }
> >      ofpbuf_uninit(&bands);
> >
> > @@ -1090,7 +1098,8 @@ ofp_to_string__(const struct ofp_header *oh,
> >          return ofp_print_meter_stats_reply(string, oh);
> >
> >      case OFPTYPE_METER_CONFIG_STATS_REPLY:
> > -        return ofp_print_meter_config_reply(string, oh);
> > +        return ofp_print_meter_config_reply(string, oh,
> > +                                            ONELINE_GET(verbosity));
>
> Previously the verbosity parameter of this function meant just now.
> Now it means oneline | verbosity.
> Above the online case is handled correctly.
> But I think there are other cases in this function where
> verbosity (the verbosity bits) need to be handled (using VERBOSITY()).
> And possibly elsewhere in this file.
>
> This is the risk of overloading an integer like this.
> We need to be very careful.
>
Right. Will do.

> >
> >      case OFPTYPE_METER_FEATURES_STATS_REPLY:
> >          return ofp_print_meter_features_reply(string, oh);
> > @@ -1278,7 +1287,7 @@ ofp_to_string(const void *oh_, size_t len,
> >              ofp_print_error(&string, error);
> >          }
> >
> > -        if (verbosity >= 5 || error) {
> > +        if (VERBOSITY(verbosity) >= 5 || error) {
> >              add_newline(&string);
> >              ds_put_hex_dump(&string, oh, len, 0, true);
> >          }
> > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> > index baab60a22..cbfd02ea6 100644
> > --- a/tests/dpif-netdev.at
> > +++ b/tests/dpif-netdev.at
> > @@ -283,11 +283,6 @@ AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
> >
> >  AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps burst stats bands=type=drop rate=1 burst_size=1'])
> >  AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=2 kbps burst stats bands=type=drop rate=1 burst_size=2'])
> > -AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=1 action=meter:1,7'])
> > -AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=7 action=meter:2,1'])
> > -AT_CHECK([ovs-ofctl add-flow br1 'in_port=2 action=8'])
> > -AT_CHECK([ovs-ofctl add-flow br1 'in_port=8 action=2'])
> > -ovs-appctl time/stop
>
> I'm confused about why this hunk is needed.
>
If I comment at the right place, these are tests for meter's operations.
test dump-meters' format with option 'oneline' both for multiple
entries and one entry.
test add-meters from a file, which is exactly what ovs-save will do.
test del-meter, make sure it works.

> >
> >  AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl
> >  OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> > @@ -298,6 +293,45 @@ 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
> > +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> > +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
> > +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> > +meter=1 pktps burst stats bands= type=drop rate=1 burst_size=1
> > +])
> > +
> > +AT_CHECK([ovs-ofctl del-meters -O openflow13 br0])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl
> > +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> > +])
> > +
> > +AT_DATA([meters.txt], [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
> > +meter=3 kbps burst stats bands= type=drop rate=2 burst_size=3
> > +])
> > +
> > +AT_CHECK([ovs-ofctl add-meters -O openflow13 br0 meters.txt])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 --oneline], [0], [dnl
> > +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> > +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=3 kbps burst stats bands= type=drop rate=2 burst_size=3
> > +])
> > +
> > +AT_CHECK([ovs-ofctl del-meters -O openflow13 br0 meter=3])
> > +
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=1 action=meter:1,7'])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=7 action=meter:2,1'])
> > +AT_CHECK([ovs-ofctl add-flow br1 'in_port=2 action=8'])
> > +AT_CHECK([ovs-ofctl add-flow br1 'in_port=8 action=2'])
> > +ovs-appctl time/stop
> > +
> >  ovs-appctl time/warp 5000
> >  for i in `seq 1 7`; do
> >    AT_CHECK(
>
> ...
>
> > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
>
> ...
>
> nit: please consider reverse xmas tree for local variables -
>      longest line to shortest.
>
OK.

> > +
> > +    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,28 +4199,36 @@ 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)
> >  {
> > +    if (oneline) {
> > +        verbosity = ONELINE_SET(verbosity);
> > +    }
>
> It feels like this should be handled in parse_options().
> It's a global (to this file) value.
> And the oneline bit needs to be handled generically.
> So it seems logical that it should also be set in general way.
>
OK.

> >      ofctl_meter_request__(ctx->argv[1], ctx->argc > 2 ? ctx->argv[2] : NULL,
> >                            OFPUTIL_METER_CONFIG);
> >  }
>
> ...
>
> > 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' \
>
> Maybe some tests for ovs-save would be useful at some point.
For add-meters, tests are already added in tests/dpif-netdev.at.
diff mbox series

Patch

diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h
index 6776eae87..a8ee2d61d 100644
--- a/include/openvswitch/ofp-meter.h
+++ b/include/openvswitch/ofp-meter.h
@@ -17,6 +17,7 @@ 
 #ifndef OPENVSWITCH_OFP_METER_H
 #define OPENVSWITCH_OFP_METER_H 1
 
+#include <stdbool.h>
 #include "openflow/openflow.h"
 #include "openvswitch/ofp-protocol.h"
 
@@ -61,7 +62,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 *,
+                                 bool oneline);
 
 struct ofputil_meter_mod {
     uint16_t command;
@@ -79,6 +81,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/include/openvswitch/ofp-print.h b/include/openvswitch/ofp-print.h
index d76f06872..cd7261c4b 100644
--- a/include/openvswitch/ofp-print.h
+++ b/include/openvswitch/ofp-print.h
@@ -38,6 +38,16 @@  struct dp_packet;
 extern "C" {
 #endif
 
+/* manipulate higher bits in verbosity for other usage */
+#define ONELINE_BIT 7
+#define ONELINE_MASK (1 << ONELINE_BIT)
+#define VERBOSITY_MASK (~ONELINE_MASK)
+
+#define VERBOSITY(verbosity) (verbosity & VERBOSITY_MASK)
+
+#define ONELINE_SET(verbosity) (verbosity | ONELINE_MASK)
+#define ONELINE_GET(verbosity) (verbosity & ONELINE_MASK)
+
 void ofp_print(FILE *, const void *, size_t, const struct ofputil_port_map *,
                const struct ofputil_table_map *, int verbosity);
 void ofp_print_packet(FILE *stream, const void *data,
diff --git a/lib/ofp-meter.c b/lib/ofp-meter.c
index 9ea40a0bf..6d760620d 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,8 @@  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,
+                            bool oneline)
 {
     uint16_t i;
 
@@ -354,6 +356,7 @@  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 ? " ": "\n");
         ofputil_format_meter_band(s, mc->flags, &mc->bands[i]);
     }
     ds_put_char(s, '\n');
@@ -578,6 +581,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 +627,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 +831,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, 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,
+                                        &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..a9cba444b 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -54,6 +54,7 @@ 
 #include "openvswitch/ofp-monitor.h"
 #include "openvswitch/ofp-msgs.h"
 #include "openvswitch/ofp-port.h"
+#include "openvswitch/ofp-print.h"
 #include "openvswitch/ofp-queue.h"
 #include "openvswitch/ofp-switch.h"
 #include "openvswitch/ofp-table.h"
@@ -365,12 +366,17 @@  ofp_print_meter_features_reply(struct ds *s, const struct ofp_header *oh)
 }
 
 static enum ofperr
-ofp_print_meter_config_reply(struct ds *s, const struct ofp_header *oh)
+ofp_print_meter_config_reply(struct ds *s, const struct ofp_header *oh,
+                             bool oneline)
 {
     struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length));
     struct ofpbuf bands;
     int retval;
 
+    if (oneline) {
+        ds_put_char(s, '\n');
+    }
+
     ofpbuf_init(&bands, 64);
     for (;;) {
         struct ofputil_meter_config mc;
@@ -379,8 +385,10 @@  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);
+        if (!oneline) {
+            ds_put_char(s, '\n');
+        }
+        ofputil_format_meter_config(s, &mc, oneline ? true : false);
     }
     ofpbuf_uninit(&bands);
 
@@ -1090,7 +1098,8 @@  ofp_to_string__(const struct ofp_header *oh,
         return ofp_print_meter_stats_reply(string, oh);
 
     case OFPTYPE_METER_CONFIG_STATS_REPLY:
-        return ofp_print_meter_config_reply(string, oh);
+        return ofp_print_meter_config_reply(string, oh,
+                                            ONELINE_GET(verbosity));
 
     case OFPTYPE_METER_FEATURES_STATS_REPLY:
         return ofp_print_meter_features_reply(string, oh);
@@ -1278,7 +1287,7 @@  ofp_to_string(const void *oh_, size_t len,
             ofp_print_error(&string, error);
         }
 
-        if (verbosity >= 5 || error) {
+        if (VERBOSITY(verbosity) >= 5 || error) {
             add_newline(&string);
             ds_put_hex_dump(&string, oh, len, 0, true);
         }
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index baab60a22..cbfd02ea6 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -283,11 +283,6 @@  AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
 
 AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps burst stats bands=type=drop rate=1 burst_size=1'])
 AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=2 kbps burst stats bands=type=drop rate=1 burst_size=2'])
-AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=1 action=meter:1,7'])
-AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=7 action=meter:2,1'])
-AT_CHECK([ovs-ofctl add-flow br1 'in_port=2 action=8'])
-AT_CHECK([ovs-ofctl add-flow br1 'in_port=8 action=2'])
-ovs-appctl time/stop
 
 AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl
 OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
@@ -298,6 +293,45 @@  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
+OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
+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
+OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
+meter=1 pktps burst stats bands= type=drop rate=1 burst_size=1
+])
+
+AT_CHECK([ovs-ofctl del-meters -O openflow13 br0])
+AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl
+OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
+])
+
+AT_DATA([meters.txt], [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
+meter=3 kbps burst stats bands= type=drop rate=2 burst_size=3
+])
+
+AT_CHECK([ovs-ofctl add-meters -O openflow13 br0 meters.txt])
+AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 --oneline], [0], [dnl
+OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
+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=3 kbps burst stats bands= type=drop rate=2 burst_size=3
+])
+
+AT_CHECK([ovs-ofctl del-meters -O openflow13 br0 meter=3])
+
+AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=1 action=meter:1,7'])
+AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=7 action=meter:2,1'])
+AT_CHECK([ovs-ofctl add-flow br1 'in_port=2 action=8'])
+AT_CHECK([ovs-ofctl add-flow br1 'in_port=8 action=2'])
+ovs-appctl time/stop
+
 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
 \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
 \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..e8a356b6e 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 bool oneline = false;
+
 static const struct ovs_cmdl_command *get_all_commands(void);
 
 OVS_NO_RETURN static void usage(void);
@@ -217,6 +220,7 @@  parse_options(int argc, char *argv[])
         OPT_COLOR,
         OPT_MAY_CREATE,
         OPT_READ_ONLY,
+        OPT_ONELINE,
         DAEMON_OPTION_ENUMS,
         OFP_VERSION_OPTION_ENUMS,
         VLOG_OPTION_ENUMS,
@@ -245,6 +249,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, NULL, OPT_ONELINE},
         DAEMON_LONG_OPTIONS,
         OFP_VERSION_LONG_OPTIONS,
         VLOG_LONG_OPTIONS,
@@ -314,6 +319,10 @@  parse_options(int argc, char *argv[])
             ovs_cmdl_print_options(long_options);
             exit(EXIT_SUCCESS);
 
+        case OPT_ONELINE:
+            oneline = true;
+            break;
+
         case OPT_BUNDLE:
             bundle = true;
             break;
@@ -475,9 +484,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 +525,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 +1454,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 +4091,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) {
+    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);
+        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);
         }
-    } 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
-ofctl_meter_request__(const char *bridge, const char *str,
-                      enum ofputil_meter_request_type type)
+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,28 +4199,36 @@  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)
 {
+    if (oneline) {
+        verbosity = ONELINE_SET(verbosity);
+    }
     ofctl_meter_request__(ctx->argv[1], ctx->argc > 2 ? ctx->argv[2] : NULL,
                           OFPUTIL_METER_CONFIG);
 }
@@ -5068,15 +5137,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' \