diff mbox series

[ovs-dev,v3] netdev-native-tnl: Fix alignment for erspan index.

Message ID 1527011904-100961-1-git-send-email-dlu998@gmail.com
State Superseded
Headers show
Series [ovs-dev,v3] netdev-native-tnl: Fix alignment for erspan index. | expand

Commit Message

Darrell Ball May 22, 2018, 5:58 p.m. UTC 
Flagged by clang.

CC: Greg Rose <gvrose8192@gmail.com>
Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
Signed-off-by: Darrell Ball <dlu998@gmail.com>
---
 lib/netdev-native-tnl.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

Gregory Rose May 22, 2018, 7:56 p.m. UTC | #1 
On 5/22/2018 10:58 AM, Darrell Ball wrote:
> Flagged by clang.
>
> CC: Greg Rose <gvrose8192@gmail.com>
> Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
> Signed-off-by: Darrell Ball <dlu998@gmail.com>

Adding William...

- Greg

> ---
>   lib/netdev-native-tnl.c | 15 ++++++---------
>   1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index c97491e..dd36695 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -653,24 +653,21 @@ netdev_erspan_build_header(const struct netdev *netdev,
>       }
>   
>       if (erspan_ver == 1) {
> -        ovs_be32 *index;
> -
>           greh->protocol = htons(ETH_TYPE_ERSPAN1);
>           greh->flags = htons(GRE_SEQ);
>           ersh->ver = 1;
>           set_sid(ersh, sid);
>   
> -        put_16aligned_be32(ALIGNED_CAST(ovs_16aligned_be32 *, ersh + 1),
> -                           htonl(tnl_cfg->erspan_idx));
> -
> -        index = (ovs_be32 *)(ersh + 1);
> -
> +        void *erspan_hdr_idx = ersh + 1;
> +        ovs_be32 erspan_idx;
>           if (tnl_cfg->erspan_idx_flow) {
> -            *index = htonl(params->flow->tunnel.erspan_idx);
> +            erspan_idx = htonl(params->flow->tunnel.erspan_idx);
>           } else {
> -            *index = htonl(tnl_cfg->erspan_idx);
> +            erspan_idx = htonl(tnl_cfg->erspan_idx);
>           }
>   
> +        memcpy(erspan_hdr_idx, &erspan_idx, sizeof erspan_idx);
> +
>           hlen = ERSPAN_GREHDR_LEN + sizeof *ersh + ERSPAN_V1_MDSIZE;
>       } else if (erspan_ver == 2) {
>           struct erspan_md2 *md2 = ALIGNED_CAST(struct erspan_md2 *, ersh + 1);
Darrell Ball May 22, 2018, 10:23 p.m. UTC | #2 
In case it helps, the Clang complaint is:

../lib/netdev-native-tnl.c:666:17: error: cast from 'struct erspan_base_hdr *' to 'ovs_be32 *' (aka 'unsigned int *') increases required alignment
      from 1 to 4 [-Werror,-Wcast-align]
        index = (ovs_be32 *)(ersh + 1);
                ^~~~~~~~~~~~~~~~~~~~~~
1 error generated.


On 5/22/18, 1:02 PM, "ovs-dev-bounces@openvswitch.org on behalf of Gregory Rose" <ovs-dev-bounces@openvswitch.org on behalf of gvrose8192@gmail.com> wrote:

    On 5/22/2018 10:58 AM, Darrell Ball wrote:
    > Flagged by clang.
    >
    > CC: Greg Rose <gvrose8192@gmail.com>
    > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
    > Signed-off-by: Darrell Ball <dlu998@gmail.com>
    
    Adding William...
    
    - Greg
    
    > ---
    >   lib/netdev-native-tnl.c | 15 ++++++---------
    >   1 file changed, 6 insertions(+), 9 deletions(-)
    >
    > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
    > index c97491e..dd36695 100644
    > --- a/lib/netdev-native-tnl.c
    > +++ b/lib/netdev-native-tnl.c
    > @@ -653,24 +653,21 @@ netdev_erspan_build_header(const struct netdev *netdev,
    >       }
    >   
    >       if (erspan_ver == 1) {
    > -        ovs_be32 *index;
    > -
    >           greh->protocol = htons(ETH_TYPE_ERSPAN1);
    >           greh->flags = htons(GRE_SEQ);
    >           ersh->ver = 1;
    >           set_sid(ersh, sid);
    >   
    > -        put_16aligned_be32(ALIGNED_CAST(ovs_16aligned_be32 *, ersh + 1),
    > -                           htonl(tnl_cfg->erspan_idx));
    > -
    > -        index = (ovs_be32 *)(ersh + 1);
    > -
    > +        void *erspan_hdr_idx = ersh + 1;
    > +        ovs_be32 erspan_idx;
    >           if (tnl_cfg->erspan_idx_flow) {
    > -            *index = htonl(params->flow->tunnel.erspan_idx);
    > +            erspan_idx = htonl(params->flow->tunnel.erspan_idx);
    >           } else {
    > -            *index = htonl(tnl_cfg->erspan_idx);
    > +            erspan_idx = htonl(tnl_cfg->erspan_idx);
    >           }
    >   
    > +        memcpy(erspan_hdr_idx, &erspan_idx, sizeof erspan_idx);
    > +
    >           hlen = ERSPAN_GREHDR_LEN + sizeof *ersh + ERSPAN_V1_MDSIZE;
    >       } else if (erspan_ver == 2) {
    >           struct erspan_md2 *md2 = ALIGNED_CAST(struct erspan_md2 *, ersh + 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=miXF3VWl6y1KFf8laCUbKv3uh9wYqNgLYz6eXwRZ-dE&s=3hbIsX3jW6GD1pUd4KHPE_x-aQCzSDlYqt62FvJ16Zs&e=
Ben Pfaff May 23, 2018, 6:27 p.m. UTC | #3 
On Tue, May 22, 2018 at 10:58:24AM -0700, Darrell Ball wrote:
> Flagged by clang.
> 
> CC: Greg Rose <gvrose8192@gmail.com>
> Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> ---
>  lib/netdev-native-tnl.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index c97491e..dd36695 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -653,24 +653,21 @@ netdev_erspan_build_header(const struct netdev *netdev,
>      }
>  
>      if (erspan_ver == 1) {
> -        ovs_be32 *index;
> -
>          greh->protocol = htons(ETH_TYPE_ERSPAN1);
>          greh->flags = htons(GRE_SEQ);
>          ersh->ver = 1;
>          set_sid(ersh, sid);
>  
> -        put_16aligned_be32(ALIGNED_CAST(ovs_16aligned_be32 *, ersh + 1),
> -                           htonl(tnl_cfg->erspan_idx));
> -
> -        index = (ovs_be32 *)(ersh + 1);
> -
> +        void *erspan_hdr_idx = ersh + 1;
> +        ovs_be32 erspan_idx;
>          if (tnl_cfg->erspan_idx_flow) {
> -            *index = htonl(params->flow->tunnel.erspan_idx);
> +            erspan_idx = htonl(params->flow->tunnel.erspan_idx);
>          } else {
> -            *index = htonl(tnl_cfg->erspan_idx);
> +            erspan_idx = htonl(tnl_cfg->erspan_idx);
>          }
>  
> +        memcpy(erspan_hdr_idx, &erspan_idx, sizeof erspan_idx);
> +

Thanks for the fix.  This looks like it was probably my ham-fisted
mistake.

The following is closer to what I would traditionally do.

What do you think?

diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index c97491ed0cc5..8a83ba82299a 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -653,24 +653,16 @@ netdev_erspan_build_header(const struct netdev *netdev,
     }
 
     if (erspan_ver == 1) {
-        ovs_be32 *index;
-
         greh->protocol = htons(ETH_TYPE_ERSPAN1);
         greh->flags = htons(GRE_SEQ);
         ersh->ver = 1;
         set_sid(ersh, sid);
 
+        uint32_t index = (tnl_cfg->erspan_idx_flow
+                          ? params->flow->tunnel.erspan_idx
+                          : tnl_cfg->erspan_idx);
         put_16aligned_be32(ALIGNED_CAST(ovs_16aligned_be32 *, ersh + 1),
-                           htonl(tnl_cfg->erspan_idx));
-
-        index = (ovs_be32 *)(ersh + 1);
-
-        if (tnl_cfg->erspan_idx_flow) {
-            *index = htonl(params->flow->tunnel.erspan_idx);
-        } else {
-            *index = htonl(tnl_cfg->erspan_idx);
-        }
-
+                           htonl(index));
         hlen = ERSPAN_GREHDR_LEN + sizeof *ersh + ERSPAN_V1_MDSIZE;
     } else if (erspan_ver == 2) {
         struct erspan_md2 *md2 = ALIGNED_CAST(struct erspan_md2 *, ersh + 1);
Darrell Ball May 24, 2018, 2:31 a.m. UTC | #4 
On Wed, May 23, 2018 at 11:27 AM, Ben Pfaff <blp@ovn.org> wrote:

> On Tue, May 22, 2018 at 10:58:24AM -0700, Darrell Ball wrote:
> > Flagged by clang.
> >
> > CC: Greg Rose <gvrose8192@gmail.com>
> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
> > Signed-off-by: Darrell Ball <dlu998@gmail.com>
> > ---
> >  lib/netdev-native-tnl.c | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> > index c97491e..dd36695 100644
> > --- a/lib/netdev-native-tnl.c
> > +++ b/lib/netdev-native-tnl.c
> > @@ -653,24 +653,21 @@ netdev_erspan_build_header(const struct netdev
> *netdev,
> >      }
> >
> >      if (erspan_ver == 1) {
> > -        ovs_be32 *index;
> > -
> >          greh->protocol = htons(ETH_TYPE_ERSPAN1);
> >          greh->flags = htons(GRE_SEQ);
> >          ersh->ver = 1;
> >          set_sid(ersh, sid);
> >
> > -        put_16aligned_be32(ALIGNED_CAST(ovs_16aligned_be32 *, ersh +
> 1),
> > -                           htonl(tnl_cfg->erspan_idx));
> > -
> > -        index = (ovs_be32 *)(ersh + 1);
> > -
> > +        void *erspan_hdr_idx = ersh + 1;
> > +        ovs_be32 erspan_idx;
> >          if (tnl_cfg->erspan_idx_flow) {
> > -            *index = htonl(params->flow->tunnel.erspan_idx);
> > +            erspan_idx = htonl(params->flow->tunnel.erspan_idx);
> >          } else {
> > -            *index = htonl(tnl_cfg->erspan_idx);
> > +            erspan_idx = htonl(tnl_cfg->erspan_idx);
> >          }
> >
> > +        memcpy(erspan_hdr_idx, &erspan_idx, sizeof erspan_idx);
> > +
>
> Thanks for the fix.  This looks like it was probably my ham-fisted
> mistake.
>
> The following is closer to what I would traditionally do.
>
> What do you think?
>


Nicer; thanks.
I am going to have to make up for not using the ternary operator recently;
maybe a 5 levels of ternary operator nesting :-).

I folded in your suggestion into a V4; I also added a second patch that was
in the works.
The V4 series is here:

https://patchwork.ozlabs.org/project/openvswitch/list/?series=46259



>
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index c97491ed0cc5..8a83ba82299a 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -653,24 +653,16 @@ netdev_erspan_build_header(const struct netdev
> *netdev,
>      }
>
>      if (erspan_ver == 1) {
> -        ovs_be32 *index;
> -
>          greh->protocol = htons(ETH_TYPE_ERSPAN1);
>          greh->flags = htons(GRE_SEQ);
>          ersh->ver = 1;
>          set_sid(ersh, sid);
>
> +        uint32_t index = (tnl_cfg->erspan_idx_flow
> +                          ? params->flow->tunnel.erspan_idx
> +                          : tnl_cfg->erspan_idx);
>          put_16aligned_be32(ALIGNED_CAST(ovs_16aligned_be32 *, ersh + 1),
> -                           htonl(tnl_cfg->erspan_idx));
> -
> -        index = (ovs_be32 *)(ersh + 1);
> -
> -        if (tnl_cfg->erspan_idx_flow) {
> -            *index = htonl(params->flow->tunnel.erspan_idx);
> -        } else {
> -            *index = htonl(tnl_cfg->erspan_idx);
> -        }
> -
> +                           htonl(index));
>          hlen = ERSPAN_GREHDR_LEN + sizeof *ersh + ERSPAN_V1_MDSIZE;
>      } else if (erspan_ver == 2) {
>          struct erspan_md2 *md2 = ALIGNED_CAST(struct erspan_md2 *, ersh +
> 1);
>
diff mbox series

Patch

diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index c97491e..dd36695 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -653,24 +653,21 @@  netdev_erspan_build_header(const struct netdev *netdev,
     }
 
     if (erspan_ver == 1) {
-        ovs_be32 *index;
-
         greh->protocol = htons(ETH_TYPE_ERSPAN1);
         greh->flags = htons(GRE_SEQ);
         ersh->ver = 1;
         set_sid(ersh, sid);
 
-        put_16aligned_be32(ALIGNED_CAST(ovs_16aligned_be32 *, ersh + 1),
-                           htonl(tnl_cfg->erspan_idx));
-
-        index = (ovs_be32 *)(ersh + 1);
-
+        void *erspan_hdr_idx = ersh + 1;
+        ovs_be32 erspan_idx;
         if (tnl_cfg->erspan_idx_flow) {
-            *index = htonl(params->flow->tunnel.erspan_idx);
+            erspan_idx = htonl(params->flow->tunnel.erspan_idx);
         } else {
-            *index = htonl(tnl_cfg->erspan_idx);
+            erspan_idx = htonl(tnl_cfg->erspan_idx);
         }
 
+        memcpy(erspan_hdr_idx, &erspan_idx, sizeof erspan_idx);
+
         hlen = ERSPAN_GREHDR_LEN + sizeof *ersh + ERSPAN_V1_MDSIZE;
     } else if (erspan_ver == 2) {
         struct erspan_md2 *md2 = ALIGNED_CAST(struct erspan_md2 *, ersh + 1);