On 5/23/23 20:51, Sayali Naval (sanaval) via dev wrote: > Does anyone have any insights on the below? > ________________________________ > From: Sayali Naval (sanaval) > Sent: Friday, May 12, 2023 11:10 AM > To: d...@openvswitch.org <d...@openvswitch.org> > Subject: [ovs-dev] ovs-vsctl Bridge IPFIX enable_input_sampling, > enable_ouput_sampling and enable_tunnel_sampling returning 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 \ > other_config:enable-tunnel-sampling=true > > where the default values are: > > enable_input_sampling: true > enable_output_sampling: true > enable_tunnel_sampling: false > > But in the existing code > https://github.com/openvswitch/ovs/blob/master/vswitchd/bridge.c#L1560-L1567, > these 3 parameters take up unexpected values in some scenarios. > > Consider the following case for enable_input_sampling and > enable_output_sampling: > > 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. > > My proposed solution is as below: > > 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", true); > > Here, by removing the negation on the smap_get bool function, we make sure > the CLI value is returned as expected and by changing the 3rd parameter to > “true”, we return the default value “true” as expected.
Yeah, the negation is very strange. I'd expect the code be implemented the way you're suggesting. Not sure why it was done this way in the first place. > > Now, consider the following case for enable_tunnel_sampling: > > be_opts.enable_tunnel_sampling = smap_get_bool(&be_cfg->other_config, > "enable-tunnel-sampling", true); > > Here again the smap_get_bool function is being used (without a negation). > > This returns unexpected value for the default case (since expected value is > false and smap_get_bool function will return true) and expected value for the > case where the sampling value is passed through the CLI. > > My proposed solution is as below: > > be_opts.enable_tunnel_sampling = smap_get_bool(&be_cfg->other_config, > "enable-tunnel-sampling", false); > > Here, by changing the 3rd parameter to “false”, we return the default value > “false” as expected. Yeah, this is also weird. The option is documented in vswitchd/vswitch.xml as disabled by default, while in fact it is enabled by default. It's been this way for 9 years already. So, it might make sense to keep it enabled, but fix the documentation instead. Adrian, do you have any thoughts on all that? > > I am happy to discuss more if there are any additional thoughts around this. > If this looks like a good fix, I can go ahead and send out a PR for review. FWIW, it's better to send patches to the mailing list instead as most of our automation is based on the mailing list workflow. Best regards, Ilya Maximets. > Thanks & Regards, > > Sayali Naval _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
