Re: [ovs-dev] [PATCH v1] bridge ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling fix unexpected values

2023-07-04 Thread Eelco Chaudron 


On 4 Jul 2023, at 8:55, 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

Hi once again,

This is even more confusing, you have sent a v1 than a v2 (which had some 
formatting problems) and now you sent a v1 again. Maybe you can send a v3, so 
it’s clear to everyone which version is the latest?

Thanks,

Eelco

> 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(&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

Re: [ovs-dev] [PATCH v1] bridge ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling fix unexpected values

2023-07-03 Thread 0-day Robot 
Bleep bloop.  Greetings Sayali Naval (sanaval), I am a robot and I have tried 
out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author Sayali Naval \(sanaval\)  needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Sayali Naval 
Lines checked: 77, Warnings: 1, Errors: 1


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1] bridge ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling fix unexpected values

2023-06-30 Thread Sayali Naval (sanaval) via dev 
Thanks. I'll look through the resources and submit a new, correctly formatted 
patch soon.

From: Ilya Maximets 
Sent: Friday, June 30, 2023 3:39 AM
To: Sayali Naval (sanaval) ; d...@openvswitch.org 

Cc: i.maxim...@ovn.org 
Subject: Re: [ovs-dev] [PATCH v1] bridge ovs-vsctl Bridge IPFIX 
enable_input_sampling, enable_ouput_sampling fix unexpected values

On 6/21/23 20:43, Sayali Naval (sanaval) via dev wrote:
> A gentle reminder to take a look at this patch.

Hi.  Sorry for a late reply.  But this patch came in strangely formatted,
so it wasn't recognized as a patch by any of our systems.

The patch has extra spacing after every line that makes it not readable
by automated tools and hence not applicable.

Could you try re-sending it?  Maybe re-configure your or use a different
mail client for that, e.g. git send-email.

There are some hints in the contribution guide:
  
https://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/

And some hints on mail client configuration from a kernel guide:
  https://static.lwn.net/kerneldoc/process/email-clients.html


For example, here is how your patch looks like:
  https://mail.openvswitch.org/pipermail/ovs-dev/2023-May/405130.html

And here is an example on how the patch formatting should look like:
  https://mail.openvswitch.org/pipermail/ovs-dev/2023-June/405786.html

Best regards, Ilya Maximets.

>
> Thanks,
> Sayali Naval
> 
> From: Sayali Naval (sanaval)
> Sent: Wednesday, May 31, 2023 11:02 AM
> 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(&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 
>
> ---
>
> [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 @@ bri

Re: [ovs-dev] [PATCH v1] bridge ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling fix unexpected values

2023-06-30 Thread Ilya Maximets 
On 6/21/23 20:43, Sayali Naval (sanaval) via dev wrote:
> A gentle reminder to take a look at this patch.

Hi.  Sorry for a late reply.  But this patch came in strangely formatted,
so it wasn't recognized as a patch by any of our systems.

The patch has extra spacing after every line that makes it not readable
by automated tools and hence not applicable.

Could you try re-sending it?  Maybe re-configure your or use a different
mail client for that, e.g. git send-email.

There are some hints in the contribution guide:
  
https://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/

And some hints on mail client configuration from a kernel guide:
  https://static.lwn.net/kerneldoc/process/email-clients.html


For example, here is how your patch looks like:
  https://mail.openvswitch.org/pipermail/ovs-dev/2023-May/405130.html

And here is an example on how the patch formatting should look like:
  https://mail.openvswitch.org/pipermail/ovs-dev/2023-June/405786.html

Best regards, Ilya Maximets.

> 
> Thanks,
> Sayali Naval
> 
> From: Sayali Naval (sanaval)
> Sent: Wednesday, May 31, 2023 11:02 AM
> 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(&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 
> 
> ---
> 
> [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,
> 
> +

Re: [ovs-dev] [PATCH v1] bridge ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling fix unexpected values

2023-06-21 Thread Sayali Naval (sanaval) via dev 
A gentle reminder to take a look at this patch.

Thanks,
Sayali Naval

From: Sayali Naval (sanaval)
Sent: Wednesday, May 31, 2023 11:02 AM
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(&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 

---

[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