[ovs-dev] flow: Further refinements to flow_pop_vlan().

Message ID 20170415042541.3654-1-blp@ovn.org
State Accepted
Headers show

Commit Message

Ben Pfaff April 15, 2017, 4:25 a.m.
This may help to suppress warnings from know-it-all compilers, and it helps
to make the code clearer too.

Reported-by: Darrell Ball <dlu998@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
Sorry, I didn't see v2 of the memset patch until I'd already applied v1.
Here's my version of this patch.

 lib/flow.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

Eric Garver April 17, 2017, 3:23 p.m. | #1
On Fri, Apr 14, 2017 at 09:25:41PM -0700, Ben Pfaff wrote:
> This may help to suppress warnings from know-it-all compilers, and it helps
> to make the code clearer too.
> 
> Reported-by: Darrell Ball <dlu998@gmail.com>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
> Sorry, I didn't see v2 of the memset patch until I'd already applied v1.
> Here's my version of this patch.
> 
>  lib/flow.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/flow.c b/lib/flow.c
> index 2b1ec4fed7ef..9d2ff93e89e4 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -2205,16 +2205,17 @@ void
>  flow_pop_vlan(struct flow *flow, struct flow_wildcards *wc)
>  {
>      int n = flow_count_vlan_headers(flow);
> -    if (n == 0) {
> -        return;
> +    if (n > 1) {
> +        if (wc) {
> +            memset(&wc->masks.vlans[1], 0xff,
> +                   sizeof(union flow_vlan_hdr) * (n - 1));
> +        }
> +        memmove(&flow->vlans[0], &flow->vlans[1],
> +                sizeof(union flow_vlan_hdr) * (n - 1));
>      }
> -    if (wc) {
> -        memset(&wc->masks.vlans[1], 0xff,
> -               sizeof(union flow_vlan_hdr) * (n - 1));
> +    if (n > 0) {
> +        memset(&flow->vlans[n - 1], 0, sizeof(union flow_vlan_hdr));
>      }
> -    memmove(&flow->vlans[0], &flow->vlans[1],
> -            sizeof(union flow_vlan_hdr) * (n - 1));
> -    memset(&flow->vlans[n - 1], 0, sizeof(union flow_vlan_hdr));
>  }
>  
>  void
> -- 
> 2.10.2

Thanks Ben and Darrell!

Acked-by: Eric Garver <e@erig.me>
Ben Pfaff April 21, 2017, 10:48 p.m. | #2
On Mon, Apr 17, 2017 at 11:23:43AM -0400, Eric Garver wrote:
> On Fri, Apr 14, 2017 at 09:25:41PM -0700, Ben Pfaff wrote:
> > This may help to suppress warnings from know-it-all compilers, and it helps
> > to make the code clearer too.
> > 
> > Reported-by: Darrell Ball <dlu998@gmail.com>
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> > Sorry, I didn't see v2 of the memset patch until I'd already applied v1.
> > Here's my version of this patch.
> > 
> >  lib/flow.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/flow.c b/lib/flow.c
> > index 2b1ec4fed7ef..9d2ff93e89e4 100644
> > --- a/lib/flow.c
> > +++ b/lib/flow.c
> > @@ -2205,16 +2205,17 @@ void
> >  flow_pop_vlan(struct flow *flow, struct flow_wildcards *wc)
> >  {
> >      int n = flow_count_vlan_headers(flow);
> > -    if (n == 0) {
> > -        return;
> > +    if (n > 1) {
> > +        if (wc) {
> > +            memset(&wc->masks.vlans[1], 0xff,
> > +                   sizeof(union flow_vlan_hdr) * (n - 1));
> > +        }
> > +        memmove(&flow->vlans[0], &flow->vlans[1],
> > +                sizeof(union flow_vlan_hdr) * (n - 1));
> >      }
> > -    if (wc) {
> > -        memset(&wc->masks.vlans[1], 0xff,
> > -               sizeof(union flow_vlan_hdr) * (n - 1));
> > +    if (n > 0) {
> > +        memset(&flow->vlans[n - 1], 0, sizeof(union flow_vlan_hdr));
> >      }
> > -    memmove(&flow->vlans[0], &flow->vlans[1],
> > -            sizeof(union flow_vlan_hdr) * (n - 1));
> > -    memset(&flow->vlans[n - 1], 0, sizeof(union flow_vlan_hdr));
> >  }
> >  
> >  void
> > -- 
> > 2.10.2
> 
> Thanks Ben and Darrell!
> 
> Acked-by: Eric Garver <e@erig.me>

Thanks!  I applied this to master.

Patch

diff --git a/lib/flow.c b/lib/flow.c
index 2b1ec4fed7ef..9d2ff93e89e4 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -2205,16 +2205,17 @@  void
 flow_pop_vlan(struct flow *flow, struct flow_wildcards *wc)
 {
     int n = flow_count_vlan_headers(flow);
-    if (n == 0) {
-        return;
+    if (n > 1) {
+        if (wc) {
+            memset(&wc->masks.vlans[1], 0xff,
+                   sizeof(union flow_vlan_hdr) * (n - 1));
+        }
+        memmove(&flow->vlans[0], &flow->vlans[1],
+                sizeof(union flow_vlan_hdr) * (n - 1));
     }
-    if (wc) {
-        memset(&wc->masks.vlans[1], 0xff,
-               sizeof(union flow_vlan_hdr) * (n - 1));
+    if (n > 0) {
+        memset(&flow->vlans[n - 1], 0, sizeof(union flow_vlan_hdr));
     }
-    memmove(&flow->vlans[0], &flow->vlans[1],
-            sizeof(union flow_vlan_hdr) * (n - 1));
-    memset(&flow->vlans[n - 1], 0, sizeof(union flow_vlan_hdr));
 }
 
 void