On Wed, Mar 01, 2023 at 04:05:17PM +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>
>
> ---
> 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 | 8 +-
> lib/ofp-meter.c | 103 +++++++++++-
> lib/ofp-print.c | 3 +-
> tests/dpif-netdev.at | 9 +
> utilities/ovs-ofctl.8.in | 25 ++-
> utilities/ovs-ofctl.c | 286 ++++++++++++++++++++++++++++----
> utilities/ovs-save | 8 +
> 7 files changed, 397 insertions(+), 45 deletions(-)
Hi Wan Junjie,
thanks for your patch.
It is a longish patch.
It might be nice to break it up a bit.
> diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h
> index 6776eae87..1bc91464e 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);
I think that:
1. Function declarations should include parameter names.
2. bool would be a better type for the new oneline parameter
void ofputil_format_meter_config(struct ds *,
const struct ofputil_meter_config *,
bool oneline);
> struct ofputil_meter_mod {
> uint16_t command;
> @@ -79,6 +80,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/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");
I think that as oneline is a boolean this may be clearer:
ds_put_cstr(s, oneline ? " ": "\n");
> ofputil_format_meter_band(s, mc->flags, &mc->bands[i]);
> }
> - ds_put_char(s, '\n');
Likewise,
if (!oneline) {
> + 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);
nit: If the type of the oneline argument becomes bool, then:
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,
nit: '[ *n_mms]' -> '[*n_mms]'
> + &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..9a25f45ea 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -379,8 +379,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);
nit: If the type of the oneline argument becomes bool, then:
ofputil_format_meter_config(s, &mm->meter, false);
> }
> ofpbuf_uninit(&bands);
>
> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> index baab60a22..5382eed0b 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
> +])
> +
Perhaps it is good to also test the restore function,
if that is not already being done.
> 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
Are there backwards compatibility concerns here...
> \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
... and here?
> \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..b9578d682 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 int oneline = 0;
> +
nit: bool would be a better type for 'online'.
Likewise, there could be a cleanup patch for similar variables
that are currently of type int.
> static const struct ovs_cmdl_command *get_all_commands(void);
>
> OVS_NO_RETURN static void usage(void);
> @@ -245,6 +248,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,
> @@ -475,9 +479,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 +520,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 +1449,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 +4086,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) {
> - char *error;
> - error = parse_ofp_meter_mod_str(&mm, str, 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;
> + 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);
> }
>
> - 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)
> +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;
> + 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);
> + }
> + ofctl_meter_mod__(argv[1], &mm, 1, usable_protocols);
> + }
> +}
nit: blank line
> +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,32 +4194,190 @@ 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)
> +{
> + 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 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;
> +
> + vconn = prepare_dump_meters(bridge, str, &mm, &protocol);
> + version = ofputil_protocol_to_ofp_version(protocol);
> + struct ofpbuf *request = ofputil_encode_meter_request(version,
> + OFPUTIL_METER_CONFIG, mm.meter.meter_id);
The logic in the two 'paragraphs' above seems to largely duplicate
ofctl_dump_meters__() => ofctl_meter_request__().
Could those functions be parameterised over oneline?
> +
> + 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);
The above, ofputil_format_meter_config(), and recv_meter_stats_reply()
seems to be a lot of code to customise what was otherwise a call to
dump_transaction().
Perhaps it would be cleaner to parameterise ofp_print() over 'oneline'.
> + free(mm.meter.bands);
> + vconn_close(vconn);
> + }
> +}
> +
> static void
> ofctl_meter_stats(struct ovs_cmdl_context *ctx)
> {
> @@ -5068,15 +5282,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' \
> --
> 2.39.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
