Re: [ovs-dev] [External] Re: [PATCH v4] utilities/ofctl: add-meters for save and restore
On Tue, Mar 07, 2023 at 12:24:55PM +0800, Wan Junjie wrote:
> Hi Simon Horman,
>
> Thanks for your review.
>
>
> On Mon, Mar 6, 2023 at 11:23 PM Simon Horman
> wrote:
> > On Sat, Mar 04, 2023 at 11:23:50AM +0800, Wan Junjie wrote:
...
> > > On Fri, Mar 3, 2023 at 11:06 PM Simon Horman
> > > wrote:
> > > > On Wed, Mar 01, 2023 at 04:05:17PM +0800, Wan Junjie wrote:
...
> > > > > +struct ofputil_meter_config *mcs;
> > > > > +size_t n_mtrs;
> > > > > +run(vconn_dump_meters(vconn, request, , _mtrs),
> > > > > +"dump meters");
> > > > > +
> > > > > +struct ds s = DS_EMPTY_INITIALIZER;
> > > > > +for (size_t i = 0; i < n_mtrs; i ++) {
> > > > > +ds_clear();
> > > > > +ofputil_format_meter_config(, [i], 1);
> > > > > +printf("%s\n", ds_cstr());
> > > > > +}
> > > > > +ds_destroy();
> > > > > +
> > > > > +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'.
Thanks. I agree that overloading/reusing the verbosity parameter
is worth a shot.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [External] Re: [PATCH v4] utilities/ofctl: add-meters for save and restore
Hi Simon Horman,
Thanks for your review.
On Mon, Mar 6, 2023 at 11:23 PM Simon Horman 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
> > 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(, 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, , );
> > > > +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
Re: [ovs-dev] [External] Re: [PATCH v4] utilities/ofctl: add-meters for save and restore
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
> 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(, 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, , );
> > > +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;
> > > +
Re: [ovs-dev] [External] Re: [PATCH v4] utilities/ofctl: add-meters for save and restore
Hi Simon,
Thanks for the review.
On Fri, Mar 3, 2023 at 11:06 PM Simon Horman wrote:
>
> On Wed, Mar 01, 2023 at 04:05:17PM +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
> >
> > ---
> > 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 | 8 +-
> > lib/ofp-meter.c | 103 +++-
> > lib/ofp-print.c | 3 +-
> > tests/dpif-netdev.at| 9 +
> > utilities/ovs-ofctl.8.in| 25 ++-
> > utilities/ovs-ofctl.c | 286
> > utilities/ovs-save | 8 +
> > 7 files changed, 397 insertions(+), 45 deletions(-)
>
> Hi Wan Junjie,
>
> thanks for your patch.
>
> It is a longish patch.
> It might be nice to break it up a bit.
>
> > diff --git a/include/openvswitch/ofp-meter.h
> > b/include/openvswitch/ofp-meter.h
> > index 6776eae87..1bc91464e 100644
> > --- a/include/openvswitch/ofp-meter.h
> > +++ b/include/openvswitch/ofp-meter.h
> > @@ -61,7 +61,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 *,
> > + int);
>
> I think that:
>
> 1. Function declarations should include parameter names.
> 2. bool would be a better type for the new oneline parameter
>
> void ofputil_format_meter_config(struct ds *,
> const struct ofputil_meter_config *,
> bool oneline);
>
OK.
>
> > struct ofputil_meter_mod {
> > uint16_t command;
> > @@ -79,6 +80,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/lib/ofp-meter.c b/lib/ofp-meter.c
> > index 9ea40a0bf..b94cb6a05 100644
> > --- a/lib/ofp-meter.c
> > +++ b/lib/ofp-meter.c
> > @@ -15,6 +15,7 @@
> > */
> >
> > #include
> > +#include
> > #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,7 @@ 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, int
> > oneline)
> > {
> > uint16_t i;
> >
> > @@ -354,9 +355,12 @@ 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 > 0 ? " ": "\n");
>
> I think that as oneline is a boolean this may be clearer:
>
> ds_put_cstr(s, oneline ? " ": "\n");
>
OK
> > ofputil_format_meter_band(s, mc->flags, >bands[i]);
> > }
> > -ds_put_char(s, '\n');
>
> Likewise,
>
> if (!oneline) {
>
> > +if (oneline == 0) {
> > +ds_put_char(s, '\n');
> > +}
> > }
> >
> > static enum ofperr
> > @@ -578,6 +582,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)) {
> > +
