On 5/6/23 13:43, 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 <[email protected]>
>
> ---
> v11:
> add OFP_ prefix to macro
> add testcases for meters
>
> 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 ++++++++--
> tests/ovs-ofctl.at | 70 ++++++++++++++++
> utilities/ovs-ofctl.8.in | 25 +++++-
> utilities/ovs-ofctl.c | 140 ++++++++++++++++++++++++--------
> utilities/ovs-save | 8 ++
> 9 files changed, 374 insertions(+), 52 deletions(-)
Hi. Thanks for the new version! It looks fine in general,
I just have a few small comments.
Please, add a record to the NEWS file regarding a new command
for ovs-ofctl.
Some other comments inline.
Best regards, Ilya Maximets.
>
> 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..fbaac2bea 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 */
I guess you missed my comment on this in the previous review.
Comments should start with a capital letter and end with a period.
> +#define OFP_ONELINE_BIT 7
> +#define OFP_ONELINE_MASK (1 << OFP_ONELINE_BIT)
> +#define OFP_VERBOSITY_MASK (~OFP_ONELINE_MASK)
> +
> +#define OFP_VERBOSITY(verbosity) (verbosity & OFP_VERBOSITY_MASK)
> +
> +#define OFP_ONELINE_SET(verbosity) (verbosity | OFP_ONELINE_MASK)
> +#define OFP_ONELINE_GET(verbosity) (verbosity & OFP_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");
There is a missing space between '" "' and the ':'.
> 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);
> + }
Don't check for a NULL pointer. free(NULL) is a valid operation.
> + }
> + 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;
> }
...
> @@ -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++) {
Indentation is off.
> + mm = &mms[i];
> + request = ofputil_encode_meter_mod(version, mm);
> + transact_noreply(vconn, request);
> + free(mm->meter.bands);
> + }
> +
> + vconn_close(vconn);
> +}
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
