On Fri, Mar 10, 2023 at 09:03:48PM +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 <[email protected]>
>
> ---
> v5:
> merge oneline to verbosity higher bits
> remove duplicate dump_meters code
Sorry for the delay.
This is looking much better to my eyes.
I've provided some more feedback inline.
...
> diff --git a/include/openvswitch/ofp-print.h b/include/openvswitch/ofp-print.h
> index d76f06872..fdcb244b3 100644
> --- a/include/openvswitch/ofp-print.h
> +++ b/include/openvswitch/ofp-print.h
> @@ -38,6 +38,15 @@ struct dp_packet;
> extern "C" {
> #endif
>
> +/* manipulate higher bits in verbosity for other usage */
> +#define ONELINE_BIT 7
> +#define VERBOSITY_MASK (1 << ONELINE_BIT)
> +
> +#define VERBOSITY(verbosity) (verbosity & ~VERBOSITY_MASK)
> +
> +#define ONELINE_SET(verbosity) (verbosity | (1 << ONELINE_BIT))
> +#define ONELINE_GET(verbosity) (verbosity & (1 << ONELINE_BIT))
Maybe:
#define ONELINE_BIT 7
#define ONELINE_MASK (1 << ONELINE_BIT)
#define VERBOSITY_MASK (~ONLINE_MASK)
#define VERBOSITY(verbosity) (verbosity & VERBOSITY_MASK)
#define ONELINE_SET(verbosity) (verbosity | ONELINE_MASK)
#define ONELINE_GET(verbosity) (verbosity & ONELINE_MASK)
...
> diff --git a/lib/ofp-meter.c b/lib/ofp-meter.c
...
> @@ -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)
> +{
This appears to largely duplicate parse_ofp_group_mod_file().
Could shared code be used?
> + 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..00473aeea 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -365,11 +365,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,
> + int verbosity)
If verbosity is otherwise unused in this function, perhaps you could consider:
ofp_print_meter_config_reply(struct ds *s, const struct ofp_header *oh,
bool oneline)
And then use ONELINE_GET(verbosity) in the caller.
> {
> struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length));
> struct ofpbuf bands;
> int retval;
> + bool oneline = ONELINE_GET(verbosity);
nit: I'd prefer if local variables were arranged in reverse xmas tree
order - longest line to shortest.
> +
> + if (oneline) {
> + ds_put_char(s, '\n');
> + }
>
> ofpbuf_init(&bands, 64);
> for (;;) {
> @@ -379,8 +385,10 @@ ofp_print_meter_config_reply(struct ds *s, const struct
> ofp_header *oh)
> if (retval) {
> break;
> }
> - ds_put_char(s, '\n');
> - ofputil_format_meter_config(s, &mc);
> + if (!oneline) {
> + ds_put_char(s, '\n');
> + }
> + ofputil_format_meter_config(s, &mc, oneline ? true : false);
> }
> ofpbuf_uninit(&bands);
>
> @@ -1090,7 +1098,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, verbosity);
I think there are other users of 'verbosity' in ofp_to_string__().
If so, those usages probably need to be updated to VERBOSITY(verbosity).
>
> case OFPTYPE_METER_FEATURES_STATS_REPLY:
> return ofp_print_meter_features_reply(string, oh);
> @@ -1278,6 +1286,7 @@ ofp_to_string(const void *oh_, size_t len,
> ofp_print_error(&string, error);
> }
>
> + verbosity = VERBOSITY(verbosity);
> if (verbosity >= 5 || error) {
nit: I think you can just do this:
if (VERBOSITY(verbosity) >= 5 || error) {
> add_newline(&string);
> ds_put_hex_dump(&string, oh, len, 0, true);
...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
