Re: [ovs-dev] [PATCH] netdev-vport: Do not log empty warnings on success.

[Daniele Di Proietto]

On 12/01/2017 09:33, "Ben Pfaff"  wrote:

>On Thu, Jan 12, 2017 at 12:23:55AM -0800, Daniele Di Proietto wrote:
>> set_tunnel_config() always logs a warning, even on success. This
>> shouldn't happen.
>> 
>> Without this, some unit tests fail.
>> 
>> Fixes: 9fff138ec3a6("netdev: Add 'errp' to set_config().")
>> Signed-off-by: Daniele Di Proietto 
>> ---
>>  lib/netdev-vport.c | 10 ++
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>> 
>> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
>> index ad5ffcc81..2db51df72 100644
>> --- a/lib/netdev-vport.c
>> +++ b/lib/netdev-vport.c
>> @@ -561,10 +561,12 @@ set_tunnel_config(struct netdev *dev_, const struct 
>> smap *args, char **errp)
>>  err = 0;
>>  
>>  out:
>> -ds_chomp(, '\n');
>> -VLOG_WARN("%s", ds_cstr());
>> -if (err) {
>> -*errp = ds_steal_cstr();
>> +if (*ds_cstr()) {
>
>How about "if (errors.length)" instead?

Ok

>
>> +ds_chomp(, '\n');
>> +VLOG_WARN("%s", ds_cstr());
>> +if (err) {
>> +*errp = ds_steal_cstr();
>> +}
>>  }
>>  
>>  ds_destroy();
>
>Acked-by: Ben Pfaff 

Thanks, pushed to master
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev-vport: Do not log empty warnings on success.

[Fischetti, Antonio]

Hi Daniele,
I've checked that without this patch I was getting
ERROR: 2316 tests were run,
75 failed unexpectedly.
2 tests were skipped.

Instead after applying this patch I get
ERROR: 2316 tests were run,
42 failed unexpectedly.
2 tests were skipped.


In particular, after I apply this patch the following tunnel tests are not 
failing anymore.

===
tunnel

749: tunnel - input  ok
750: tunnel - ECN decapsulation  ok
751: tunnel - output ok
752: tunnel - unencrypted tunnel and not setting skb_mark ok
753: tunnel - unencrypted tunnel and setting skb_mark to 1 ok
754: tunnel - unencrypted tunnel and setting skb_mark to 2 ok
755: tunnel - ToS and TTL inheritanceok
756: tunnel - set_tunnel ok
757: tunnel - keyok
758: tunnel - key match  ok
759: tunnel - Geneve ok
760: tunnel - VXLAN  ok
761: tunnel - LISP   ok
762: tunnel - different VXLAN UDP port   ok
763: ofproto-dpif - set_field - tun_src/tun_dst/tun_id ok
764: tunnel - Geneve metadataok
765: tunnel - Geneve option present  ok
766: tunnel - concomitant IPv6 and IPv4 tunnels  ok

tunnel_push_pop

767: tunnel_push_pop - actionok
768: tunnel_push_pop - packet_outok

tunnel_push_pop_ipv6

769: tunnel_push_pop_ipv6 - action   ok

1093: ofproto-dpif - truncate and output to gre tunnel ok

1097: ofproto-dpif - sFlow packet sampling - tunnel set ok
1098: ofproto-dpif - sFlow packet sampling - tunnel push ok

1107: ofproto-dpif - Flow IPFIX sanity check - tunnel set ok

1147: ofproto-dpif megaflow - tunnels ok

1154: ofproto-dpif - ofproto-dpif-monitor 1   ok
1155: ofproto-dpif - ofproto-dpif-monitor 2   ok

===

One further comment, this patch fails with ./utilities/checkpatch.py.
E: No signatures found.
Warnings: 0, Errors: 1

Besides this, it looks ok to me.

Acked-by: Antonio Fischetti <antonio.fische...@intel.com>


> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Daniele Di Proietto
> Sent: Thursday, January 12, 2017 8:24 AM
> To: d...@openvswitch.org
> Cc: Daniele Di Proietto <diproiet...@vmware.com>
> Subject: [ovs-dev] [PATCH] netdev-vport: Do not log empty warnings on
> success.
> 
> set_tunnel_config() always logs a warning, even on success. This
> shouldn't happen.
> 
> Without this, some unit tests fail.
> 
> Fixes: 9fff138ec3a6("netdev: Add 'errp' to set_config().")
> Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com>
> ---
>  lib/netdev-vport.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index ad5ffcc81..2db51df72 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -561,10 +561,12 @@ set_tunnel_config(struct netdev *dev_, const struct
> smap *args, char **errp)
>  err = 0;
> 
>  out:
> -ds_chomp(, '\n');
> -VLOG_WARN("%s", ds_cstr());
> -if (err) {
> -*errp = ds_steal_cstr();
> +if (*ds_cstr()) {
> +ds_chomp(, '\n');
> +VLOG_WARN("%s", ds_cstr());
> +if (err) {
> +*errp = ds_steal_cstr();
> +}
>  }
> 
>  ds_destroy();
> --
> 2.11.0
> 
> ___
> 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

[ovs-dev] [PATCH] netdev-vport: Do not log empty warnings on success.

[Daniele Di Proietto]

set_tunnel_config() always logs a warning, even on success. This
shouldn't happen.

Without this, some unit tests fail.

Fixes: 9fff138ec3a6("netdev: Add 'errp' to set_config().")
Signed-off-by: Daniele Di Proietto 
---
 lib/netdev-vport.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index ad5ffcc81..2db51df72 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -561,10 +561,12 @@ set_tunnel_config(struct netdev *dev_, const struct smap 
*args, char **errp)
 err = 0;
 
 out:
-ds_chomp(, '\n');
-VLOG_WARN("%s", ds_cstr());
-if (err) {
-*errp = ds_steal_cstr();
+if (*ds_cstr()) {
+ds_chomp(, '\n');
+VLOG_WARN("%s", ds_cstr());
+if (err) {
+*errp = ds_steal_cstr();
+}
 }
 
 ds_destroy();
-- 
2.11.0

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