| Message ID | 20170112082355.103138-1-diproiettod@vmware.com |
|---|---|
| State | Accepted |
| Headers | show |
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 inheritance ok 756: tunnel - set_tunnel ok 757: tunnel - key ok 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 metadata ok 765: tunnel - Geneve option present ok 766: tunnel - concomitant IPv6 and IPv4 tunnels ok tunnel_push_pop 767: tunnel_push_pop - action ok 768: tunnel_push_pop - packet_out ok 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.fischetti@intel.com> > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Daniele Di Proietto > Sent: Thursday, January 12, 2017 8:24 AM > To: dev@openvswitch.org > Cc: Daniele Di Proietto <diproiettod@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 <diproiettod@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(&errors, '\n'); > - VLOG_WARN("%s", ds_cstr(&errors)); > - if (err) { > - *errp = ds_steal_cstr(&errors); > + if (*ds_cstr(&errors)) { > + ds_chomp(&errors, '\n'); > + VLOG_WARN("%s", ds_cstr(&errors)); > + if (err) { > + *errp = ds_steal_cstr(&errors); > + } > } > > ds_destroy(&errors); > -- > 2.11.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 <diproiettod@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(&errors, '\n'); > - VLOG_WARN("%s", ds_cstr(&errors)); > - if (err) { > - *errp = ds_steal_cstr(&errors); > + if (*ds_cstr(&errors)) { How about "if (errors.length)" instead? > + ds_chomp(&errors, '\n'); > + VLOG_WARN("%s", ds_cstr(&errors)); > + if (err) { > + *errp = ds_steal_cstr(&errors); > + } > } > > ds_destroy(&errors); Acked-by: Ben Pfaff <blp@ovn.org>
On 12/01/2017 09:33, "Ben Pfaff" <blp@ovn.org> 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 <diproiettod@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(&errors, '\n'); >> - VLOG_WARN("%s", ds_cstr(&errors)); >> - if (err) { >> - *errp = ds_steal_cstr(&errors); >> + if (*ds_cstr(&errors)) { > >How about "if (errors.length)" instead? Ok > >> + ds_chomp(&errors, '\n'); >> + VLOG_WARN("%s", ds_cstr(&errors)); >> + if (err) { >> + *errp = ds_steal_cstr(&errors); >> + } >> } >> >> ds_destroy(&errors); > >Acked-by: Ben Pfaff <blp@ovn.org> Thanks, pushed to master
On 12/01/2017 03:40, "Fischetti, Antonio" <antonio.fischetti@intel.com> wrote: >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. I'm curious why these fail for you, they happen to pass on travis or locally. Maybe this is something worth investigating. > > >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 inheritance ok >756: tunnel - set_tunnel ok >757: tunnel - key ok >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 metadata ok >765: tunnel - Geneve option present ok >766: tunnel - concomitant IPv6 and IPv4 tunnels ok > >tunnel_push_pop > >767: tunnel_push_pop - action ok >768: tunnel_push_pop - packet_out ok > >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 Mmmh, not sure why it's reporting this error. There is a signoff line. I tried to run it and it didn't report anything. > >Besides this, it looks ok to me. > >Acked-by: Antonio Fischetti <antonio.fischetti@intel.com> Thanks for the review > > >> -----Original Message----- >> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- >> bounces@openvswitch.org] On Behalf Of Daniele Di Proietto >> Sent: Thursday, January 12, 2017 8:24 AM >> To: dev@openvswitch.org >> Cc: Daniele Di Proietto <diproiettod@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 <diproiettod@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(&errors, '\n'); >> - VLOG_WARN("%s", ds_cstr(&errors)); >> - if (err) { >> - *errp = ds_steal_cstr(&errors); >> + if (*ds_cstr(&errors)) { >> + ds_chomp(&errors, '\n'); >> + VLOG_WARN("%s", ds_cstr(&errors)); >> + if (err) { >> + *errp = ds_steal_cstr(&errors); >> + } >> } >> >> ds_destroy(&errors); >> -- >> 2.11.0 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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(&errors, '\n'); - VLOG_WARN("%s", ds_cstr(&errors)); - if (err) { - *errp = ds_steal_cstr(&errors); + if (*ds_cstr(&errors)) { + ds_chomp(&errors, '\n'); + VLOG_WARN("%s", ds_cstr(&errors)); + if (err) { + *errp = ds_steal_cstr(&errors); + } } ds_destroy(&errors);
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 <diproiettod@vmware.com> --- lib/netdev-vport.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)