Re: [ovs-dev] [PATCH] ipfix: trigger revalidation if ipfix options changes
On Fri, Apr 22, 2022 at 11:41:25AM +0200, Eelco Chaudron wrote:
>
>
> On 16 Apr 2022, at 13:09, lic121 wrote:
>
> > ipfix cfg creation/deleting triggers revalidation. But this does
> > not cover the case where ipfix options changes, which also suppose
> > to trigger revalidation.
>
> Some small comments below, and I think we should add a test case to verify
> the cases that could yield a reconfigure.
Thanks for the review, will address the commnents in the next version.
>
> > Fixes: a9f5ee1199e1 ("ofproto-dpif: Trigger revalidation when ipfix config
> > set.")
> > Signed-off-by: lic121
> > ---
> > ofproto/ofproto-dpif-ipfix.c | 46
> >
> > ofproto/ofproto-dpif-ipfix.h | 2 +-
> > ofproto/ofproto-dpif.c | 7 +++
> > 3 files changed, 33 insertions(+), 22 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > index fc927fe..7d316ca 100644
> > --- a/ofproto/ofproto-dpif-ipfix.c
> > +++ b/ofproto/ofproto-dpif-ipfix.c
> > @@ -926,17 +926,19 @@ dpif_ipfix_bridge_exporter_destroy(struct
> > dpif_ipfix_bridge_exporter *exporter)
> > static void
> > dpif_ipfix_bridge_exporter_set_options(
> > struct dpif_ipfix_bridge_exporter *exporter,
> > -const struct ofproto_ipfix_bridge_exporter_options *options)
> > +const struct ofproto_ipfix_bridge_exporter_options *options,
> > +bool *options_changed)
> > {
> > -bool options_changed;
> > -
> > if (!options || sset_is_empty(>targets)) {
> > /* No point in doing any work if there are no targets. */
> > -dpif_ipfix_bridge_exporter_clear(exporter);
> > +if (exporter->options) {
> > +dpif_ipfix_bridge_exporter_clear(exporter);
> > +*options_changed = true;
> > +}
>
> This can return without the option changed being set, so it should become:
>
> } else {
>*options_changed = false;
> }
>
> > return;
> > }
> >
> > -options_changed = (
> > +*options_changed = (
> > !exporter->options
> > || !ofproto_ipfix_bridge_exporter_options_equal(
> > options, exporter->options));
> > @@ -945,7 +947,7 @@ dpif_ipfix_bridge_exporter_set_options(
> > * shortchanged in collectors (which indicates that opening one or
> > * more of the configured collectors failed, so that we should
> > * retry). */
> > -if (options_changed
> > +if (*options_changed
> > || collectors_count(exporter->exporter.collectors)
> > < sset_count(>targets)) {
> > if (!dpif_ipfix_exporter_set_options(
> > @@ -957,7 +959,7 @@ dpif_ipfix_bridge_exporter_set_options(
> > }
> >
> > /* Avoid reconfiguring if options didn't change. */
> > -if (!options_changed) {
> > +if (!*options_changed) {
> > return;
> > }
> >
> > @@ -1015,17 +1017,19 @@ dpif_ipfix_flow_exporter_destroy(struct
> > dpif_ipfix_flow_exporter *exporter)
> > static bool
> > dpif_ipfix_flow_exporter_set_options(
> > struct dpif_ipfix_flow_exporter *exporter,
> > -const struct ofproto_ipfix_flow_exporter_options *options)
> > +const struct ofproto_ipfix_flow_exporter_options *options,
> > +bool *options_changed)
> > {
> > -bool options_changed;
> > -
> > if (sset_is_empty(>targets)) {
> > /* No point in doing any work if there are no targets. */
> > -dpif_ipfix_flow_exporter_clear(exporter);
> > +if (exporter->options) {
> > +dpif_ipfix_flow_exporter_clear(exporter);
> > +*options_changed = true;
> > +}
>
> This can return without the option changed being set, so it should become:
>
> } else {
>*options_changed = false;
> }
>
> > return true;
> > }
> >
> > -options_changed = (
> > +*options_changed = (
> > !exporter->options
> > || !ofproto_ipfix_flow_exporter_options_equal(
> > options, exporter->options));
> > @@ -1034,7 +1038,7 @@ dpif_ipfix_flow_exporter_set_options(
> > * shortchanged in collectors (which indicates that opening one or
> > * more of the configured collectors failed, so that we should
> > * retry). */
> > -if (options_changed
> > +if (*options_changed
> > || collectors_count(exporter->exporter.collectors)
> > < sset_count(>targets)) {
> > if (!dpif_ipfix_exporter_set_options(
> > @@ -1046,7 +1050,7 @@ dpif_ipfix_flow_exporter_set_options(
> > }
> >
> > /* Avoid reconfiguring if options didn't change. */
> > -if (!options_changed) {
> > +if (!*options_changed) {
> > return true;
> > }
> >
> > @@ -1069,7 +1073,7 @@ remove_flow_exporter(struct dpif_ipfix *di,
> > free(node);
> > }
> >
> > -void
> > +bool
> > dpif_ipfix_set_options(
> > struct dpif_ipfix *di,
> > const struct ofproto_ipfix_bridge_exporter_options
> > *bridge_exporter_options,
> > @@ -1077,12
Re: [ovs-dev] [PATCH] ipfix: trigger revalidation if ipfix options changes
On 16 Apr 2022, at 13:09, lic121 wrote:
> ipfix cfg creation/deleting triggers revalidation. But this does
> not cover the case where ipfix options changes, which also suppose
> to trigger revalidation.
Some small comments below, and I think we should add a test case to verify the
cases that could yield a reconfigure.
> Fixes: a9f5ee1199e1 ("ofproto-dpif: Trigger revalidation when ipfix config
> set.")
> Signed-off-by: lic121
> ---
> ofproto/ofproto-dpif-ipfix.c | 46
>
> ofproto/ofproto-dpif-ipfix.h | 2 +-
> ofproto/ofproto-dpif.c | 7 +++
> 3 files changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index fc927fe..7d316ca 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -926,17 +926,19 @@ dpif_ipfix_bridge_exporter_destroy(struct
> dpif_ipfix_bridge_exporter *exporter)
> static void
> dpif_ipfix_bridge_exporter_set_options(
> struct dpif_ipfix_bridge_exporter *exporter,
> -const struct ofproto_ipfix_bridge_exporter_options *options)
> +const struct ofproto_ipfix_bridge_exporter_options *options,
> +bool *options_changed)
> {
> -bool options_changed;
> -
> if (!options || sset_is_empty(>targets)) {
> /* No point in doing any work if there are no targets. */
> -dpif_ipfix_bridge_exporter_clear(exporter);
> +if (exporter->options) {
> +dpif_ipfix_bridge_exporter_clear(exporter);
> +*options_changed = true;
> +}
This can return without the option changed being set, so it should become:
} else {
*options_changed = false;
}
> return;
> }
>
> -options_changed = (
> +*options_changed = (
> !exporter->options
> || !ofproto_ipfix_bridge_exporter_options_equal(
> options, exporter->options));
> @@ -945,7 +947,7 @@ dpif_ipfix_bridge_exporter_set_options(
> * shortchanged in collectors (which indicates that opening one or
> * more of the configured collectors failed, so that we should
> * retry). */
> -if (options_changed
> +if (*options_changed
> || collectors_count(exporter->exporter.collectors)
> < sset_count(>targets)) {
> if (!dpif_ipfix_exporter_set_options(
> @@ -957,7 +959,7 @@ dpif_ipfix_bridge_exporter_set_options(
> }
>
> /* Avoid reconfiguring if options didn't change. */
> -if (!options_changed) {
> +if (!*options_changed) {
> return;
> }
>
> @@ -1015,17 +1017,19 @@ dpif_ipfix_flow_exporter_destroy(struct
> dpif_ipfix_flow_exporter *exporter)
> static bool
> dpif_ipfix_flow_exporter_set_options(
> struct dpif_ipfix_flow_exporter *exporter,
> -const struct ofproto_ipfix_flow_exporter_options *options)
> +const struct ofproto_ipfix_flow_exporter_options *options,
> +bool *options_changed)
> {
> -bool options_changed;
> -
> if (sset_is_empty(>targets)) {
> /* No point in doing any work if there are no targets. */
> -dpif_ipfix_flow_exporter_clear(exporter);
> +if (exporter->options) {
> +dpif_ipfix_flow_exporter_clear(exporter);
> +*options_changed = true;
> +}
This can return without the option changed being set, so it should become:
} else {
*options_changed = false;
}
> return true;
> }
>
> -options_changed = (
> +*options_changed = (
> !exporter->options
> || !ofproto_ipfix_flow_exporter_options_equal(
> options, exporter->options));
> @@ -1034,7 +1038,7 @@ dpif_ipfix_flow_exporter_set_options(
> * shortchanged in collectors (which indicates that opening one or
> * more of the configured collectors failed, so that we should
> * retry). */
> -if (options_changed
> +if (*options_changed
> || collectors_count(exporter->exporter.collectors)
> < sset_count(>targets)) {
> if (!dpif_ipfix_exporter_set_options(
> @@ -1046,7 +1050,7 @@ dpif_ipfix_flow_exporter_set_options(
> }
>
> /* Avoid reconfiguring if options didn't change. */
> -if (!options_changed) {
> +if (!*options_changed) {
> return true;
> }
>
> @@ -1069,7 +1073,7 @@ remove_flow_exporter(struct dpif_ipfix *di,
> free(node);
> }
>
> -void
> +bool
> dpif_ipfix_set_options(
> struct dpif_ipfix *di,
> const struct ofproto_ipfix_bridge_exporter_options
> *bridge_exporter_options,
> @@ -1077,12 +1081,14 @@ dpif_ipfix_set_options(
> size_t n_flow_exporters_options) OVS_EXCLUDED(mutex)
> {
> int i;
> +bool beo_changed, feo_changed, entry_changed;
I’m not a compiler but looking at the sequence below I think feo_changed needs
to be initialized to false.
Wondering why we do not get an error on this…
> struct ofproto_ipfix_flow_exporter_options *options;
> struct
[ovs-dev] [PATCH] ipfix: trigger revalidation if ipfix options changes
ipfix cfg creation/deleting triggers revalidation. But this does
not cover the case where ipfix options changes, which also suppose
to trigger revalidation.
Fixes: a9f5ee1199e1 ("ofproto-dpif: Trigger revalidation when ipfix config
set.")
Signed-off-by: lic121
---
ofproto/ofproto-dpif-ipfix.c | 46
ofproto/ofproto-dpif-ipfix.h | 2 +-
ofproto/ofproto-dpif.c | 7 +++
3 files changed, 33 insertions(+), 22 deletions(-)
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index fc927fe..7d316ca 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -926,17 +926,19 @@ dpif_ipfix_bridge_exporter_destroy(struct
dpif_ipfix_bridge_exporter *exporter)
static void
dpif_ipfix_bridge_exporter_set_options(
struct dpif_ipfix_bridge_exporter *exporter,
-const struct ofproto_ipfix_bridge_exporter_options *options)
+const struct ofproto_ipfix_bridge_exporter_options *options,
+bool *options_changed)
{
-bool options_changed;
-
if (!options || sset_is_empty(>targets)) {
/* No point in doing any work if there are no targets. */
-dpif_ipfix_bridge_exporter_clear(exporter);
+if (exporter->options) {
+dpif_ipfix_bridge_exporter_clear(exporter);
+*options_changed = true;
+}
return;
}
-options_changed = (
+*options_changed = (
!exporter->options
|| !ofproto_ipfix_bridge_exporter_options_equal(
options, exporter->options));
@@ -945,7 +947,7 @@ dpif_ipfix_bridge_exporter_set_options(
* shortchanged in collectors (which indicates that opening one or
* more of the configured collectors failed, so that we should
* retry). */
-if (options_changed
+if (*options_changed
|| collectors_count(exporter->exporter.collectors)
< sset_count(>targets)) {
if (!dpif_ipfix_exporter_set_options(
@@ -957,7 +959,7 @@ dpif_ipfix_bridge_exporter_set_options(
}
/* Avoid reconfiguring if options didn't change. */
-if (!options_changed) {
+if (!*options_changed) {
return;
}
@@ -1015,17 +1017,19 @@ dpif_ipfix_flow_exporter_destroy(struct
dpif_ipfix_flow_exporter *exporter)
static bool
dpif_ipfix_flow_exporter_set_options(
struct dpif_ipfix_flow_exporter *exporter,
-const struct ofproto_ipfix_flow_exporter_options *options)
+const struct ofproto_ipfix_flow_exporter_options *options,
+bool *options_changed)
{
-bool options_changed;
-
if (sset_is_empty(>targets)) {
/* No point in doing any work if there are no targets. */
-dpif_ipfix_flow_exporter_clear(exporter);
+if (exporter->options) {
+dpif_ipfix_flow_exporter_clear(exporter);
+*options_changed = true;
+}
return true;
}
-options_changed = (
+*options_changed = (
!exporter->options
|| !ofproto_ipfix_flow_exporter_options_equal(
options, exporter->options));
@@ -1034,7 +1038,7 @@ dpif_ipfix_flow_exporter_set_options(
* shortchanged in collectors (which indicates that opening one or
* more of the configured collectors failed, so that we should
* retry). */
-if (options_changed
+if (*options_changed
|| collectors_count(exporter->exporter.collectors)
< sset_count(>targets)) {
if (!dpif_ipfix_exporter_set_options(
@@ -1046,7 +1050,7 @@ dpif_ipfix_flow_exporter_set_options(
}
/* Avoid reconfiguring if options didn't change. */
-if (!options_changed) {
+if (!*options_changed) {
return true;
}
@@ -1069,7 +1073,7 @@ remove_flow_exporter(struct dpif_ipfix *di,
free(node);
}
-void
+bool
dpif_ipfix_set_options(
struct dpif_ipfix *di,
const struct ofproto_ipfix_bridge_exporter_options
*bridge_exporter_options,
@@ -1077,12 +1081,14 @@ dpif_ipfix_set_options(
size_t n_flow_exporters_options) OVS_EXCLUDED(mutex)
{
int i;
+bool beo_changed, feo_changed, entry_changed;
struct ofproto_ipfix_flow_exporter_options *options;
struct dpif_ipfix_flow_exporter_map_node *node;
ovs_mutex_lock();
dpif_ipfix_bridge_exporter_set_options(>bridge_exporter,
- bridge_exporter_options);
+ bridge_exporter_options,
+ _changed);
/* Add new flow exporters and update current flow exporters. */
options = (struct ofproto_ipfix_flow_exporter_options *)
@@ -1095,10 +1101,14 @@ dpif_ipfix_set_options(
dpif_ipfix_flow_exporter_init(>exporter);
hmap_insert(>flow_exporter_map, >node,
hash_int(options->collector_set_id, 0));
+feo_changed = true;
}
-if (!dpif_ipfix_flow_exporter_set_options(>exporter, options)) {
+
