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 <simon.hor...@corigine.com>
> 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.
> > > \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.
> Regards,
> Wan Junjie
...
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
