Hi Simon,
On Wed, Mar 29, 2023 at 11:57 PM Simon Horman <[email protected]> wrote: > > 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) > > ... > OK > > 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? > They don't share structures like ofputil_meter_mod. An option is to union them or make a generic function with void pointer. But this may be a bad idea, like parse_ofp_group_mod_file did not reuse code from parse_ofp_flow_mod_file, we may change meter's structure in the future. Then the functions will diff a lot and be hard to manage in one function. If we want to make all the ofp structure reuse a common code then a huge refactor to the ofp level will be needed. > > + 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. OK > > > + > > + 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). > OK > > > > 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) { > OK Regards, Wan > > 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
