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 <wanjun...@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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
