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(&be_cfg->other_config, > "enable-input-sampling", false); > > be_opts.enable_output_sampling = !smap_get_bool(&be_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\) <sana...@cisco.com> 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(&be_cfg->other_config, > "enable-tunnel-sampling", true); > > - be_opts.enable_input_sampling = !smap_get_bool(&be_cfg->other_config, > - "enable-input-sampling", > false); > + be_opts.enable_input_sampling = smap_get_bool(&be_cfg->other_config, > + "enable-input-sampling", true); > > - be_opts.enable_output_sampling = > !smap_get_bool(&be_cfg->other_config, > - "enable-output-sampling", > false); > + be_opts.enable_output_sampling = smap_get_bool(&be_cfg->other_config, > + "enable-output-sampling", > true); > > virtual_obs_id = smap_get(&be_cfg->other_config, "virtual_obs_id"); > be_opts.virtual_obs_id = nullable_xstrdup(virtual_obs_id); > -- > ________________________________ > From: dev <ovs-dev-boun...@openvswitch.org> on behalf of Sayali Naval > (sanaval) via dev <ovs-dev@openvswitch.org> > Sent: Monday, July 3, 2023 4:51 PM > To: d...@openvswitch.org <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(&be_cfg->other_config, > "enable-input-sampling", false); > > be_opts.enable_output_sampling = !smap_get_bool(&be_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 <sana...@cisco.com> > --- > [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(&be_cfg->other_config, > "enable-tunnel-sampling", true); > > - be_opts.enable_input_sampling = !smap_get_bool(&be_cfg->other_config, > - "enable-input-sampling", > false); > + be_opts.enable_input_sampling = smap_get_bool(&be_cfg->other_config, > + "enable-input-sampling", true); > > - be_opts.enable_output_sampling = > !smap_get_bool(&be_cfg->other_config, > - "enable-output-sampling", > false); > + be_opts.enable_output_sampling = smap_get_bool(&be_cfg->other_config, > + "enable-output-sampling", > true); > > virtual_obs_id = smap_get(&be_cfg->other_config, "virtual_obs_id"); > be_opts.virtual_obs_id = nullable_xstrdup(virtual_obs_id); > -- > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
