[ovs-dev,patch_v1] ovs build: Fix memset with zero size warning.

Submitted by Darrell Ball on March 19, 2017, 5:11 p.m.

Details

Message ID 1489943469-42818-1-git-send-email-dlu998@gmail.com
State Superseded
Headers show

Commit Message

Darrell Ball March 19, 2017, 5:11 p.m.
In file included from /usr/include/string.h:640:0,
                 from ./lib/string.h:20,
                 from /usr/include/netinet/icmp6.h:22,
                 from ../lib/flow.h:21,
                 from ../lib/flow.c:18:
In function 'memset',
    inlined from 'flow_push_vlan_uninit' at ../lib/flow.c:2188:19:
/usr/include/x86_64-linux-gnu/bits/string3.h:81:30: error:
call to '__warn_memset_zero_len' declared with attribute warning:
memset used with constant zero length parameter; this could be
due to transposed parameters [-Werror]
       __warn_memset_zero_len ();
                              ^
cc1: all warnings being treated as errors
make[2]: *** [lib/flow.lo] Error 1

Fixes: f0fb825a3785 ("Add support for 802.1ad (QinQ tunneling)")
Signed-off-by: Darrell Ball <dlu998@gmail.com>
---
 lib/flow.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Eric Garver March 20, 2017, 3:27 p.m.
On Sun, Mar 19, 2017 at 10:11:09AM -0700, Darrell Ball wrote:
> In file included from /usr/include/string.h:640:0,
>                  from ./lib/string.h:20,
>                  from /usr/include/netinet/icmp6.h:22,
>                  from ../lib/flow.h:21,
>                  from ../lib/flow.c:18:
> In function 'memset',
>     inlined from 'flow_push_vlan_uninit' at ../lib/flow.c:2188:19:
> /usr/include/x86_64-linux-gnu/bits/string3.h:81:30: error:
> call to '__warn_memset_zero_len' declared with attribute warning:
> memset used with constant zero length parameter; this could be
> due to transposed parameters [-Werror]
>        __warn_memset_zero_len ();
>                               ^
> cc1: all warnings being treated as errors
> make[2]: *** [lib/flow.lo] Error 1
> 
> Fixes: f0fb825a3785 ("Add support for 802.1ad (QinQ tunneling)")
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> ---
>  lib/flow.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/flow.c b/lib/flow.c
> index f628526..0a67455 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -2184,7 +2184,9 @@ flow_push_vlan_uninit(struct flow *flow, struct flow_wildcards *wc)
>  {
>      if (wc) {
>          int n = flow_count_vlan_headers(flow);
> -        memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
> +        if (n) {

Better to use (n > 0) since n is of type int.

Thanks!

> +            memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
> +        }
>      }
>      memmove(&flow->vlans[1], &flow->vlans[0],
>              sizeof(union flow_vlan_hdr) * (FLOW_MAX_VLAN_HEADERS - 1));
> -- 
> 1.9.1
Darrell Ball March 20, 2017, 6:39 p.m.
On 3/20/17, 8:27 AM, "ovs-dev-bounces@openvswitch.org on behalf of Eric Garver" <ovs-dev-bounces@openvswitch.org on behalf of e@erig.me> wrote:

    On Sun, Mar 19, 2017 at 10:11:09AM -0700, Darrell Ball wrote:
    > In file included from /usr/include/string.h:640:0,

    >                  from ./lib/string.h:20,

    >                  from /usr/include/netinet/icmp6.h:22,

    >                  from ../lib/flow.h:21,

    >                  from ../lib/flow.c:18:

    > In function 'memset',

    >     inlined from 'flow_push_vlan_uninit' at ../lib/flow.c:2188:19:

    > /usr/include/x86_64-linux-gnu/bits/string3.h:81:30: error:

    > call to '__warn_memset_zero_len' declared with attribute warning:

    > memset used with constant zero length parameter; this could be

    > due to transposed parameters [-Werror]

    >        __warn_memset_zero_len ();

    >                               ^

    > cc1: all warnings being treated as errors

    > make[2]: *** [lib/flow.lo] Error 1

    > 

    > Fixes: f0fb825a3785 ("Add support for 802.1ad (QinQ tunneling)")

    > Signed-off-by: Darrell Ball <dlu998@gmail.com>

    > ---

    >  lib/flow.c | 4 +++-

    >  1 file changed, 3 insertions(+), 1 deletion(-)

    > 

    > diff --git a/lib/flow.c b/lib/flow.c

    > index f628526..0a67455 100644

    > --- a/lib/flow.c

    > +++ b/lib/flow.c

    > @@ -2184,7 +2184,9 @@ flow_push_vlan_uninit(struct flow *flow, struct flow_wildcards *wc)

    >  {

    >      if (wc) {

    >          int n = flow_count_vlan_headers(flow);

    > -        memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);

    > +        if (n) {

    
    Better to use (n > 0) since n is of type int.
    
    Thanks!

ahh , it is “int”; I auto-translated to something else.
The called API flow_count_vlan_headers cannot return negative values.
so it is cosmetic, but I agree; I also added some similar checks in the
code associated with the same commit, as it looks better to the eye.

I would prefer not using signed values for number of vlans, but this code
has been reviewed and seems correct in its usage of same, so I won’t
press the matter.

    
    > +            memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);

    > +        }

    >      }

    >      memmove(&flow->vlans[1], &flow->vlans[0],

    >              sizeof(union flow_vlan_hdr) * (FLOW_MAX_VLAN_HEADERS - 1));

    > -- 

    > 1.9.1

    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=N-gxJn1peieH0LmA-VwtdBswybNu6Qtqax8G0Yv25zQ&s=SVWxmtMJQZigGxQK8y8vqH2PlJZgxK8CIRotUzF9Kfc&e=
Eric Garver March 20, 2017, 7:27 p.m.
On Mon, Mar 20, 2017 at 06:39:15PM +0000, Darrell Ball wrote:
> 
> 
> On 3/20/17, 8:27 AM, "ovs-dev-bounces@openvswitch.org on behalf of Eric Garver" <ovs-dev-bounces@openvswitch.org on behalf of e@erig.me> wrote:
> 
>     On Sun, Mar 19, 2017 at 10:11:09AM -0700, Darrell Ball wrote:
>     > In file included from /usr/include/string.h:640:0,
>     >                  from ./lib/string.h:20,
>     >                  from /usr/include/netinet/icmp6.h:22,
>     >                  from ../lib/flow.h:21,
>     >                  from ../lib/flow.c:18:
>     > In function 'memset',
>     >     inlined from 'flow_push_vlan_uninit' at ../lib/flow.c:2188:19:
>     > /usr/include/x86_64-linux-gnu/bits/string3.h:81:30: error:
>     > call to '__warn_memset_zero_len' declared with attribute warning:
>     > memset used with constant zero length parameter; this could be
>     > due to transposed parameters [-Werror]
>     >        __warn_memset_zero_len ();
>     >                               ^
>     > cc1: all warnings being treated as errors
>     > make[2]: *** [lib/flow.lo] Error 1
>     > 
>     > Fixes: f0fb825a3785 ("Add support for 802.1ad (QinQ tunneling)")
>     > Signed-off-by: Darrell Ball <dlu998@gmail.com>
>     > ---
>     >  lib/flow.c | 4 +++-
>     >  1 file changed, 3 insertions(+), 1 deletion(-)
>     > 
>     > diff --git a/lib/flow.c b/lib/flow.c
>     > index f628526..0a67455 100644
>     > --- a/lib/flow.c
>     > +++ b/lib/flow.c
>     > @@ -2184,7 +2184,9 @@ flow_push_vlan_uninit(struct flow *flow, struct flow_wildcards *wc)
>     >  {
>     >      if (wc) {
>     >          int n = flow_count_vlan_headers(flow);
>     > -        memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
>     > +        if (n) {
>     
>     Better to use (n > 0) since n is of type int.
>     
>     Thanks!
> 
> ahh , it is “int”; I auto-translated to something else.
> The called API flow_count_vlan_headers cannot return negative values.
> so it is cosmetic, but I agree; I also added some similar checks in the
> code associated with the same commit, as it looks better to the eye.
> 
> I would prefer not using signed values for number of vlans, but this code
> has been reviewed and seems correct in its usage of same, so I won’t
> press the matter.

100% agree. This patch made me look at changing to use size_t for
flow_count_vlan_headers(), but it requires other changes. So I'll
attempt it separately. Thanks for pointing it out.

> 
>     
>     > +            memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
>     > +        }
>     >      }
>     >      memmove(&flow->vlans[1], &flow->vlans[0],
>     >              sizeof(union flow_vlan_hdr) * (FLOW_MAX_VLAN_HEADERS - 1));
>     > -- 
>     > 1.9.1
Ben Pfaff April 15, 2017, 4:18 a.m.
On Sun, Mar 19, 2017 at 10:11:09AM -0700, Darrell Ball wrote:
> In file included from /usr/include/string.h:640:0,
>                  from ./lib/string.h:20,
>                  from /usr/include/netinet/icmp6.h:22,
>                  from ../lib/flow.h:21,
>                  from ../lib/flow.c:18:
> In function 'memset',
>     inlined from 'flow_push_vlan_uninit' at ../lib/flow.c:2188:19:
> /usr/include/x86_64-linux-gnu/bits/string3.h:81:30: error:
> call to '__warn_memset_zero_len' declared with attribute warning:
> memset used with constant zero length parameter; this could be
> due to transposed parameters [-Werror]
>        __warn_memset_zero_len ();
>                               ^
> cc1: all warnings being treated as errors
> make[2]: *** [lib/flow.lo] Error 1
> 
> Fixes: f0fb825a3785 ("Add support for 802.1ad (QinQ tunneling)")
> Signed-off-by: Darrell Ball <dlu998@gmail.com>

Thanks, applied to master.

Patch hide | download patch | download mbox

diff --git a/lib/flow.c b/lib/flow.c
index f628526..0a67455 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -2184,7 +2184,9 @@  flow_push_vlan_uninit(struct flow *flow, struct flow_wildcards *wc)
 {
     if (wc) {
         int n = flow_count_vlan_headers(flow);
-        memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
+        if (n) {
+            memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
+        }
     }
     memmove(&flow->vlans[1], &flow->vlans[0],
             sizeof(union flow_vlan_hdr) * (FLOW_MAX_VLAN_HEADERS - 1));