Re: [ovs-dev] [PATCH v2] bridge ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling fix unexpected values
On 4 Jul 2023, at 3:39, Sayali Naval (sanaval) via dev wrote:
> As per the Open vSwitch Manual
> (http://www.openvswitch.org/support/dist-docs/ovs-vsctl.8.txt) the Bridge
> IPFIX parameters can be passed as follows:
>
> ovs-vsctl -- set Bridge br0 ipfix=@i \
> -- --id=@i create IPFIX targets=\"192.168.0.34:4739\" obs_do‐
> main_id=123 obs_point_id=456 cache_active_timeout=60
> cache_max_flows=13 \
> other_config:enable-input-sampling=false \
> other_config:enable-output-sampling=false
>
> where the default values are:
> enable_input_sampling: true
> enable_output_sampling: true
>
> But in the existing code
> https://github.com/openvswitch/ovs/blob/master/vswitchd/bridge.c#L1563-L1567,
> these 2 parameters take up unexpected values in some scenarios.
>
> be_opts.enable_input_sampling = !smap_get_bool(_cfg->other_config,
> "enable-input-sampling", false);
>
> be_opts.enable_output_sampling = !smap_get_bool(_cfg->other_config,
> "enable-output-sampling",
> false);
>
> Here, the function smap_get_bool is being used with a negation.
>
> smap_get_bool is defined as below:
> (https://github.com/openvswitch/ovs/blob/master/lib/smap.c#L220-L232)
>
> /* Gets the value associated with 'key' in 'smap' and converts it to a
> boolean.
> * If 'key' is not in 'smap', or its value is neither "true" nor "false",
> * returns 'def'. */
> bool
> smap_get_bool(const struct smap *smap, const char *key, bool def)
> {
> const char *value = smap_get_def(smap, key, "");
> if (def) {
> return strcasecmp("false", value) != 0;
> } else {
> return !strcasecmp("true", value);
> }
> }
>
> This returns expected values for the default case (since the above code will
> negate “false” we get from smap_get bool function and return the value
> “true”) but unexpected values for the case where the sampling value is passed
> through the CLI.
> For example, if we pass "true" for other_config:enable-input-sampling in the
> CLI, the above code will negate the “true” value we get from the smap_bool
> function and return the value “false”. Same would be the case for
> enable_output_sampling.
>
> Signed-off-by: Sayali Naval \(sanaval\)
Hi,
Thanks for your patch, however, your V2, has two patches as I guess you replied
manually to your previous patch.
In addition, you should also include the version history of your patches. This
is an example of a patch including version information (notice the location);
https://mail.openvswitch.org/pipermail/ovs-dev/2023-June/405204.html
Can you resent the patch as a new thread (not a reply to a previous version),
with only the content of the actual patch?
Thanks,
Eelco
> ---
> [bridge-ipfix-fix 277b3baae] Fix values for enable_input_sampling &
> enable_ouput_sampling
> Date: Thu May 25 10:32:43 2023 -0700
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index f5dc59ad0..b972d55d0 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -1560,11 +1560,11 @@ bridge_configure_ipfix(struct bridge *br)
> be_opts.enable_tunnel_sampling = smap_get_bool(_cfg->other_config,
> "enable-tunnel-sampling", true);
>
> -be_opts.enable_input_sampling = !smap_get_bool(_cfg->other_config,
> - "enable-input-sampling",
> false);
> +be_opts.enable_input_sampling = smap_get_bool(_cfg->other_config,
> + "enable-input-sampling", true);
>
> -be_opts.enable_output_sampling =
> !smap_get_bool(_cfg->other_config,
> - "enable-output-sampling",
> false);
> +be_opts.enable_output_sampling = smap_get_bool(_cfg->other_config,
> + "enable-output-sampling",
> true);
>
> virtual_obs_id = smap_get(_cfg->other_config, "virtual_obs_id");
> be_opts.virtual_obs_id = nullable_xstrdup(virtual_obs_id);
> --
>
> From: dev on behalf of Sayali Naval
> (sanaval) via dev
> Sent: Monday, July 3, 2023 4:51 PM
> To: d...@openvswitch.org
> Subject: [ovs-dev] [PATCH v1] bridge ovs-vsctl Bridge IPFIX
> enable_input_sampling, enable_ouput_sampling fix unexpected values
>
> As per the Open vSwitch Manual
> (http://www.openvswitch.org/support/dist-docs/ovs-vsctl.8.txt) the Bridge
> IPFIX parameters can be passed as follows:
>
> ovs-vsctl -- set Bridge br0 ipfix=@i \
> -- --id=@i create IPFIX targets=\"192.168.0.34:4739\" obs_do‐
> main_id=123 obs_point_id=456 cache_active_timeout=60
> cache_max_flows=13 \
> other_config:enable-input-sampling=false \
>
Re: [ovs-dev] [PATCH v2] bridge ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling fix unexpected values
As per the Open vSwitch Manual
(http://www.openvswitch.org/support/dist-docs/ovs-vsctl.8.txt) the Bridge IPFIX
parameters can be passed as follows:
ovs-vsctl -- set Bridge br0 ipfix=@i \
-- --id=@i create IPFIX targets=\"192.168.0.34:4739\" obs_do‐
main_id=123 obs_point_id=456 cache_active_timeout=60
cache_max_flows=13 \
other_config:enable-input-sampling=false \
other_config:enable-output-sampling=false
where the default values are:
enable_input_sampling: true
enable_output_sampling: true
But in the existing code
https://github.com/openvswitch/ovs/blob/master/vswitchd/bridge.c#L1563-L1567,
these 2 parameters take up unexpected values in some scenarios.
be_opts.enable_input_sampling = !smap_get_bool(_cfg->other_config,
"enable-input-sampling", false);
be_opts.enable_output_sampling = !smap_get_bool(_cfg->other_config,
"enable-output-sampling", false);
Here, the function smap_get_bool is being used with a negation.
smap_get_bool is defined as below:
(https://github.com/openvswitch/ovs/blob/master/lib/smap.c#L220-L232)
/* Gets the value associated with 'key' in 'smap' and converts it to a boolean.
* If 'key' is not in 'smap', or its value is neither "true" nor "false",
* returns 'def'. */
bool
smap_get_bool(const struct smap *smap, const char *key, bool def)
{
const char *value = smap_get_def(smap, key, "");
if (def) {
return strcasecmp("false", value) != 0;
} else {
return !strcasecmp("true", value);
}
}
This returns expected values for the default case (since the above code will
negate “false” we get from smap_get bool function and return the value “true”)
but unexpected values for the case where the sampling value is passed through
the CLI.
For example, if we pass "true" for other_config:enable-input-sampling in the
CLI, the above code will negate the “true” value we get from the smap_bool
function and return the value “false”. Same would be the case for
enable_output_sampling.
Signed-off-by: Sayali Naval \(sanaval\)
---
[bridge-ipfix-fix 277b3baae] Fix values for enable_input_sampling &
enable_ouput_sampling
Date: Thu May 25 10:32:43 2023 -0700
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index f5dc59ad0..b972d55d0 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1560,11 +1560,11 @@ bridge_configure_ipfix(struct bridge *br)
be_opts.enable_tunnel_sampling = smap_get_bool(_cfg->other_config,
"enable-tunnel-sampling", true);
-be_opts.enable_input_sampling = !smap_get_bool(_cfg->other_config,
- "enable-input-sampling", false);
+be_opts.enable_input_sampling = smap_get_bool(_cfg->other_config,
+ "enable-input-sampling", true);
-be_opts.enable_output_sampling = !smap_get_bool(_cfg->other_config,
- "enable-output-sampling", false);
+be_opts.enable_output_sampling = smap_get_bool(_cfg->other_config,
+ "enable-output-sampling", true);
virtual_obs_id = smap_get(_cfg->other_config, "virtual_obs_id");
be_opts.virtual_obs_id = nullable_xstrdup(virtual_obs_id);
--
From: dev on behalf of Sayali Naval (sanaval)
via dev
Sent: Monday, July 3, 2023 4:51 PM
To: d...@openvswitch.org
Subject: [ovs-dev] [PATCH v1] bridge ovs-vsctl Bridge IPFIX
enable_input_sampling, enable_ouput_sampling fix unexpected values
As per the Open vSwitch Manual
(http://www.openvswitch.org/support/dist-docs/ovs-vsctl.8.txt) the Bridge IPFIX
parameters can be passed as follows:
ovs-vsctl -- set Bridge br0 ipfix=@i \
-- --id=@i create IPFIX targets=\"192.168.0.34:4739\" obs_do‐
main_id=123 obs_point_id=456 cache_active_timeout=60
cache_max_flows=13 \
other_config:enable-input-sampling=false \
other_config:enable-output-sampling=false
where the default values are:
enable_input_sampling: true
enable_output_sampling: true
But in the existing code
https://github.com/openvswitch/ovs/blob/master/vswitchd/bridge.c#L1563-L1567,
these 2 parameters take up unexpected values in some scenarios.
be_opts.enable_input_sampling = !smap_get_bool(_cfg->other_config,
"enable-input-sampling", false);
be_opts.enable_output_sampling = !smap_get_bool(_cfg->other_config,
"enable-output-sampling", false);
Here, the function smap_get_bool is being used with a negation.
smap_get_bool is defined as below:
