Hi Simon Horman, Thanks for your review.
On Mon, Mar 6, 2023 at 11:23 PM Simon Horman <[email protected]> wrote: > > On Sat, Mar 04, 2023 at 11:23:50AM +0800, Wan Junjie wrote: > > Hi Simon, > > > > Thanks for the review. > > Hi Wan Junjie, > > Thanks for responding. > > > On Fri, Mar 3, 2023 at 11:06 PM Simon Horman <[email protected]> > > wrote: > > > On Wed, Mar 01, 2023 at 04:05:17PM +0800, Wan Junjie wrote: > > ... > > > > > 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 > > > > > > Are there backwards compatibility concerns here... > > > > > In fact, old docs are not clear on meter syntax, it means meter=id. > > Ok, so the documentation is being fixed to match the current implementation? > If so I think that should be a separate patch. > Yes, and it is pointed out by Adrián Moreno in the first reply to this patch. I will do the split in a later patch. > > > > \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 > > > > > > ... and here? > > > > > > > \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. > > > > . > > ... > > > > > +static void > > > > +ofctl_dump_meters(struct ovs_cmdl_context *ctx) > > > > +{ > > > > + if (!oneline) { > > > > + ofctl_dump_meters__(ctx); > > > > + } else { > > > > + struct ofputil_meter_mod mm; > > > > + struct vconn *vconn; > > > > + enum ofputil_protocol protocol; > > > > + enum ofp_version version; > > > > + > > > > + memset(&mm, 0, sizeof mm); > > > > + const char *bridge = ctx->argv[1]; > > > > + const char *str = ctx->argc > 2 ? ctx->argv[2] : NULL; > > > > + > > > > + vconn = prepare_dump_meters(bridge, str, &mm, &protocol); > > > > + version = ofputil_protocol_to_ofp_version(protocol); > > > > + struct ofpbuf *request = ofputil_encode_meter_request(version, > > > > + OFPUTIL_METER_CONFIG, mm.meter.meter_id); > > > > > > The logic in the two 'paragraphs' above seems to largely duplicate > > > ofctl_dump_meters__() => ofctl_meter_request__(). > > > > > > Could those functions be parameterised over oneline? > > > > > ofctl_meter_request__() will handle stats, features requests. We can > > put 'oneline' to > > parameters, but that will make other functions a little confused about > > this parameter. > > A combined function will be a little more complicated since it will > > have to judge the > > request type, changes won't save us any code. > > > > > > + > > > > + struct ofputil_meter_config *mcs; > > > > + size_t n_mtrs; > > > > + run(vconn_dump_meters(vconn, request, &mcs, &n_mtrs), > > > > + "dump meters"); > > > > + > > > > + struct ds s = DS_EMPTY_INITIALIZER; > > > > + for (size_t i = 0; i < n_mtrs; i ++) { > > > > + ds_clear(&s); > > > > + ofputil_format_meter_config(&s, &mcs[i], 1); > > > > + printf("%s\n", ds_cstr(&s)); > > > > + } > > > > + ds_destroy(&s); > > > > + > > > > + for (size_t i = 0; i < n_mtrs; i ++) { > > > > + free(CONST_CAST(struct ofputil_meter_band *, > > > > mcs[i].bands)); > > > > + } > > > > + free(mcs); > > > > > > The above, ofputil_format_meter_config(), and recv_meter_stats_reply() > > > seems to be a lot of code to customise what was otherwise a call to > > > dump_transaction(). > > > > > > Perhaps it would be cleaner to parameterise ofp_print() over 'oneline'. > > > > > That was my first thought, then I realize that I would have to add > > this parameter to several > > functions recursively, like dump_transaction(), ofp_print(), > > ofp_to_string__(), ofp_to_string(). > > I don't think these functions really need that parameter since only > > meter has compatibility issues. > > I understand your point. But I think it would be better to teach the > core/common code how to handle this and avoid open coding the special case. > > Perhaps a 'oneline' parameter isn't the best approach. > But really one bit of information is needed in order > for core/common code to implement a (slightly) different behaviour. > > And it is conceivable that other dump functions will need > alternate behaviour in future. > OK, I will see if I can merge it to 'verbosity'. > > Regards, > > Wan Junjie > > ... _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
