| Message ID | 20230411132543.45719-1-wanjunjie@bytedance.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v10] utilities/ofctl: add-meters for save and restore | expand |
| 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 |
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: 714, 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
On Tue, Apr 11, 2023 at 09:25:43PM +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> > > --- > v10: > merge oneline to verbosity in parse_options > ofp_to_string__ handle verbosity bits right Thanks, this one looks good to me. Reviewed-by: Simon Horman <simon.horman@corigine.com>
On 4/11/23 15:25, Wan Junjie via dev 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> Hi. Thanks for the patch! See some comemnts inline. Best regards, Ilya Maximets. > > --- > v10: > merge oneline to verbosity in parse_options > ofp_to_string__ handle verbosity bits right > > 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 | 20 +++-- > tests/dpif-netdev.at | 44 ++++++++-- > utilities/ovs-ofctl.8.in | 25 +++++- > utilities/ovs-ofctl.c | 140 ++++++++++++++++++++++++-------- > utilities/ovs-save | 8 ++ > 8 files changed, 304 insertions(+), 52 deletions(-) > > 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 */ Comments should start with a capital letter and end with a period. > +#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) This is a public header and the names above are too generic to be safely exported. You should add some sort of prefix. e.g. OFP_PRINT_. > + > 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..0879e705f 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); > > @@ -977,6 +985,8 @@ ofp_to_string__(const struct ofp_header *oh, > ofp_print_stats(string, oh); > } > > + bool oneline = !!ONELINE_GET(verbosity); > + verbosity = VERBOSITY(verbosity); > const void *msg = oh; > enum ofptype type = ofptype_from_ofpraw(raw); > switch (type) { > @@ -1090,7 +1100,7 @@ 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); > > case OFPTYPE_METER_FEATURES_STATS_REPLY: > return ofp_print_meter_features_reply(string, oh); > @@ -1278,7 +1288,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 The patch adds a lot of new ways of parsing and dumping meters, but this one is the only test that is covering new functionality. For exmaple, we can have 'add', 'delete', 'modify' commands in the file, but we don't have a test for this. All cases above have no bands specified. I'm curious how the oneline prints will look with bands, multiple bands per meter and if the code is actually capable of parsing multi-band dumps in a oneline form. That's, I think, the main reason why meter dumps are multi-line. Please, add tests to cover at least these cases to ovs-ofctl.at. > + > 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 24d0941cf..a5ff4b3c4 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; > @@ -390,6 +399,10 @@ parse_options(int argc, char *argv[]) > } > } > > + if (oneline) { > + verbosity = ONELINE_SET(verbosity); > + } > + > ctl_timeout_setup(timeout); > > if (n_criteria) { > @@ -475,9 +488,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" Why this change? > " 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 +529,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 +1458,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); > @@ -4084,57 +4099,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 vconn *vconn; > enum ofputil_protocol protocol; > - enum ofputil_protocol usable_protocols; > + struct ofputil_meter_mod *mm; > enum ofp_version version; > + struct ofpbuf *request; > + struct vconn *vconn; > + 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) > +{ > + enum ofputil_protocol protocol; > + struct ofputil_meter_mod mm; > + enum ofp_version version; > + struct vconn *vconn; > + > + 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)); > @@ -4142,23 +4207,28 @@ 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 > @@ -5072,15 +5142,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' \
On Thu, May 4, 2023 at 7:08 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 4/11/23 15:25, Wan Junjie via dev 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> > > Hi. Thanks for the patch! See some comemnts inline. > > Best regards, Ilya Maximets. > Hi Ilya, > > > > --- > > v10: > > merge oneline to verbosity in parse_options > > ofp_to_string__ handle verbosity bits right > > > > 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 | 20 +++-- > > tests/dpif-netdev.at | 44 ++++++++-- > > utilities/ovs-ofctl.8.in | 25 +++++- > > utilities/ovs-ofctl.c | 140 ++++++++++++++++++++++++-------- > > utilities/ovs-save | 8 ++ > > 8 files changed, 304 insertions(+), 52 deletions(-) > > > > 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 */ > > Comments should start with a capital letter and end with a period. > > > +#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) > > This is a public header and the names above are too generic to be safely > exported. You should add some sort of prefix. e.g. OFP_PRINT_. > OK > > + > > 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..0879e705f 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); > > > > @@ -977,6 +985,8 @@ ofp_to_string__(const struct ofp_header *oh, > > ofp_print_stats(string, oh); > > } > > > > + bool oneline = !!ONELINE_GET(verbosity); > > + verbosity = VERBOSITY(verbosity); > > const void *msg = oh; > > enum ofptype type = ofptype_from_ofpraw(raw); > > switch (type) { > > @@ -1090,7 +1100,7 @@ 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); > > > > case OFPTYPE_METER_FEATURES_STATS_REPLY: > > return ofp_print_meter_features_reply(string, oh); > > @@ -1278,7 +1288,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 > > The patch adds a lot of new ways of parsing and dumping meters, but this > one is the only test that is covering new functionality. > > For exmaple, we can have 'add', 'delete', 'modify' commands in the file, > but we don't have a test for this. > All cases above have no bands specified. I'm curious how the oneline > prints will look with bands, multiple bands per meter and if the code > is actually capable of parsing multi-band dumps in a oneline form. > That's, I think, the main reason why meter dumps are multi-line. > > Please, add tests to cover at least these cases to ovs-ofctl.at. > > OK. The old code will print the meter like "..bands=\ntype=..." even we have only one band here. This is the main reason we need a oneline form to process the meters saved. And your concern about multiple bands is right, they should be appended to the same line and they are. > > + > > 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 24d0941cf..a5ff4b3c4 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; > > @@ -390,6 +399,10 @@ parse_options(int argc, char *argv[]) > > } > > } > > > > + if (oneline) { > > + verbosity = ONELINE_SET(verbosity); > > + } > > + > > ctl_timeout_setup(timeout); > > > > if (n_criteria) { > > @@ -475,9 +488,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" > > Why this change? Be more specific. dump-meters with [METER] or not will print different results. > > > " 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 +529,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 +1458,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); > > @@ -4084,57 +4099,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 vconn *vconn; > > enum ofputil_protocol protocol; > > - enum ofputil_protocol usable_protocols; > > + struct ofputil_meter_mod *mm; > > enum ofp_version version; > > + struct ofpbuf *request; > > + struct vconn *vconn; > > + 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) > > +{ > > + enum ofputil_protocol protocol; > > + struct ofputil_meter_mod mm; > > + enum ofp_version version; > > + struct vconn *vconn; > > + > > + 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)); > > @@ -4142,23 +4207,28 @@ 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 > > @@ -5072,15 +5142,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' \ > Regards, Wan Junjie
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..0879e705f 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); @@ -977,6 +985,8 @@ ofp_to_string__(const struct ofp_header *oh, ofp_print_stats(string, oh); } + bool oneline = !!ONELINE_GET(verbosity); + verbosity = VERBOSITY(verbosity); const void *msg = oh; enum ofptype type = ofptype_from_ofpraw(raw); switch (type) { @@ -1090,7 +1100,7 @@ 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); case OFPTYPE_METER_FEATURES_STATS_REPLY: return ofp_print_meter_features_reply(string, oh); @@ -1278,7 +1288,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 24d0941cf..a5ff4b3c4 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; @@ -390,6 +399,10 @@ parse_options(int argc, char *argv[]) } } + if (oneline) { + verbosity = ONELINE_SET(verbosity); + } + ctl_timeout_setup(timeout); if (n_criteria) { @@ -475,9 +488,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 +529,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 +1458,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); @@ -4084,57 +4099,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 vconn *vconn; enum ofputil_protocol protocol; - enum ofputil_protocol usable_protocols; + struct ofputil_meter_mod *mm; enum ofp_version version; + struct ofpbuf *request; + struct vconn *vconn; + 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) +{ + enum ofputil_protocol protocol; + struct ofputil_meter_mod mm; + enum ofp_version version; + struct vconn *vconn; + + 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)); @@ -4142,23 +4207,28 @@ 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 @@ -5072,15 +5142,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' \
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> --- v10: merge oneline to verbosity in parse_options ofp_to_string__ handle verbosity bits right 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 | 20 +++-- tests/dpif-netdev.at | 44 ++++++++-- utilities/ovs-ofctl.8.in | 25 +++++- utilities/ovs-ofctl.c | 140 ++++++++++++++++++++++++-------- utilities/ovs-save | 8 ++ 8 files changed, 304 insertions(+), 52 deletions(-)