On Tue, Apr 11, 2023 at 7:45 PM Simon Horman <[email protected]> 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 <[email protected]>
> >
> > ---
> > 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
