Hi Adrian, Thank you for your review.
On Sat, Feb 25, 2023 at 1:17 AM Adrian Moreno <[email protected]> wrote: > > Hi Wan, > > Sorry for the delay in the review. > I've started looking at this patch. Please see some initial comments below. > I'll > continue on Monday. > > Thanks. > > On 11/15/22 05:16, 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]> > > --- > > v3: > > add '--oneline' option for dump-meter(s) command > > > > v2: > > fix failed testcases, as dump-meters format changes > > --- > > include/openvswitch/ofp-meter.h | 9 +- > > lib/ofp-meter.c | 103 ++++++++++++- > > lib/ofp-print.c | 5 +- > > tests/dpif-netdev.at | 9 ++ > > utilities/ovs-ofctl.8.in | 20 ++- > > utilities/ovs-ofctl.c | 263 +++++++++++++++++++++++++++++--- > > utilities/ovs-save | 8 + > > 7 files changed, 383 insertions(+), 34 deletions(-) > > > > diff --git a/include/openvswitch/ofp-meter.h > > b/include/openvswitch/ofp-meter.h > > index 6776eae87..0514b4ec4 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); > > > > struct ofputil_meter_mod { > > uint16_t command; > > @@ -79,6 +80,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; > > + > > Please align all the arguments one space after the "(". Also I think "int > command" fits in the first line. > > OK, I'll fix it. > > 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"); > > ofputil_format_meter_band(s, mc->flags, &mc->bands[i]); > > } > > - ds_put_char(s, '\n'); > > + 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); > > +} > > + > > +/* 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) > > +{ > > I would really like not to duplicate (actually triplicate) this function. Do > you > see an easy way to reuse one of the other almost identical functions in > ofp-group.c and ofp-flow.c? > We may add a function to do the line process part and call xxx_mod_str parser part as callback. But there is no real easy way. Will need a big patchset to cover these ofp_xxx files. > > + 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)); > > + } > > + > > This option of using stdin is not properly documented in ovs-ofctl(8). Please > document it. Will do. > > > + 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 bd37fa17a..6d7fc857e 100644 > > --- a/lib/ofp-print.c > > +++ b/lib/ofp-print.c > > @@ -378,8 +378,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); > > } > > ofpbuf_uninit(&bands); > > > > @@ -1269,7 +1270,7 @@ ofp_to_string(const void *oh_, size_t len, > > ds_put_hex_dump(&string, oh, len, 0, true); > > return ds_steal_cstr(&string); > > } > > - > > + > > Why remove the section delimiter? > Could be my editor setting did. Are these invisible characters still needed? Anyway I'll leave it here. > > static void > > print_and_free(FILE *stream, char *string) > > { > > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at > > index 6aff1eda7..2139b64c0 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 > > +]) > > + > > 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 10a6a64de..ed3444fe3 100644 > > --- a/utilities/ovs-ofctl.8.in > > +++ b/utilities/ovs-ofctl.8.in > > @@ -464,20 +464,29 @@ of OpenFlow does Open vSwitch support?'' in the Open > > vSwitch FAQ. > > 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 spicify whether a meter is to be added, modified, or deleted. > > s/spicify/specify/ Yes. Will fix. > > > +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" > > Modify an existing meter. > > . > > .IP "\fBdel\-meters \fIswitch\fR [\fImeter\fR]" > > 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]" > > 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 > > @@ -1311,6 +1320,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 by default print each band of a meter > > +in a separate line. With \fB\-\-oneline\fR, all bands will be printed > > +in a single line. This is useful for dump meters to a file and restore. > > +. > > This reads a bit strange to me (I'm not native English speaker). I'd make the > following changes: > s/command by default print each band of meter in separate line/command prints > each band in a separate line by default/ > s/This is useful for dump meters to a file and restore/This is useful for > dumping meters to a file and restoring them/ > OK. > > .IP "\fB\-\-read-only\fR" > > Do not execute read/write commands. > > . > > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c > > index fe9114580..37cd541b6 100644 > > --- a/utilities/ovs-ofctl.c > > +++ b/utilities/ovs-ofctl.c > > @@ -156,6 +156,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; > > + > > static const struct ovs_cmdl_command *get_all_commands(void); > > > > OVS_NO_RETURN static void usage(void); > > @@ -244,6 +247,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, > > @@ -474,9 +478,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] [--oneline] print METER > > entries\n" > > Suggesting this syntax in the "--help" and ovs-ofctl(8) seems to contract the > general command syntax which is: > ovs-ofctl [options] command [switch] [args...] > > For the sake of consistency across all the commands, I would either omit the > option in the short command help or at least put it in the right order. > > For the documentation (ovs-ofctl(8)), I would also put it before (similar to > how > group commands are documented). > OK, that makes sense. > Also, just realized "dump-meter" (singular) is not documented here. > There is no 'dump-meter' command. But ’dump-meters‘ can accept a meter id and print like a (singular) command. > > " 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" > > @@ -511,6 +516,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" > > @@ -4032,32 +4038,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); > > This call to memset has no point now. It's being called on a pointer. > > Now that the allocation of the "struct ofputil_meter_mod" has been moved out > of > this function, so should the memset. In particular to ofctl_meter_mod_file() > and > ofctl_meter_mod(). > Yes, nice catch. Will fix it. > > > - 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; > > + > > Extra space. > > > + } > > + error = parse_ofp_meter_mod_file(argv[2], command, > > + &mms, &n_mms, &usable_protocols); > > nit: Please an extra space so arguments are allinged. > OK, I will fix the code alignment. > > + 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 > > @@ -4090,32 +4134,201 @@ 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) > > +{ > > I'll continue next week but my first feeling is that we should try to refactor > recv_flow_stats_reply and vconn_dump_flows in lib/vconn.c so that we don't > have > to duplicate all this code again. Do you have any ideas? > I'm trying not to touch too much common code of ofp_flows in this patch. Like parse_ofp_xxx_mod_file functions, these duplicate code share the same logic but have different output parameter structures. Union the structure or add a 'type' parameter with void pointer may avoid duplicates. But all these functions could be more complicated, And the refactor may involve more code than we thought. I suggest to keep it. Is it OK ? or I should do the refactor now? > > + 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 usable_protocols; > > + 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; > > + if (str) { > > + char *error; > > + 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; > > + } > > + > > + protocol = open_vconn_for_flow_mod(bridge, &vconn, > > usable_protocols); > > + version = ofputil_protocol_to_ofp_version(protocol); > > + struct ofpbuf *request = ofputil_encode_meter_request(version, > > + OFPUTIL_METER_CONFIG, mm.meter.meter_id); > > + > > Duplicated code. How about we have a prepare_dump_meters that initializes the > vconn for both this function and ofctl_meter_request__()? > OKay. > > > + 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); > > + free(mm.meter.bands); > > + vconn_close(vconn); > > + } > > +} > > + > > static void > > ofctl_meter_stats(struct ovs_cmdl_context *ctx) > > { > > @@ -5020,15 +5233,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", > > + { "dump-meter", "switch meter [--oneline]", > > 1, 2, ofctl_dump_meters, OVS_RO }, > > - { "dump-meters", "switch", > > + { "dump-meters", "switch [meter] [--oneline]", > > 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' \ > > > > -- > Adrián Moreno > Regards, Wan Junjie _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
