[ovs-dev] ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling and enable_tunnel_sampling returning unexpected values

[Sayali Naval (sanaval) via dev]

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.



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.


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.


Thanks & Regards,

Sayali Naval
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev