[PATCH 0/13] make return of 0 explicit

[PATCH 0/13] make return of 0 explicit

[Julia Lawall]

Sometimes a local variable is used as a return value in a case where the
return value is always 0.  The result is more apparent if this variable is
just replaced by 0.  This is done by the following semantic patch, although
some cleanups may be needed. (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
local idexpression ret;
identifier reti;
expression e;
position p;
@@

ret@reti = 0;
... when != \(ret = e\|ret &= e\|ret |= e\|ret += e\|ret -= e\|ret *= e\|ret ^= e\)
    when != \(++ret\|--ret\|ret++\|ret--\)
    when != &ret
return ret;@p

@bad1 exists@
expression e != 0;
local idexpression r.ret;
position r.p;
@@

 \(ret = e\|ret &= e\|ret |= e\|ret += e\|ret -= e\|ret *= e\|ret ^= e\|++ret\|--ret\|ret++\|ret--\|&ret\)
 ...
 return ret;@p

@bad2 exists@
identifier r.reti;
position r.p;
identifier f;
type T;
@@

f(...,T reti,...) {
 ...
 return reti;@p
}

@change depends on !bad1 && !bad2@
position r.p;
local idexpression r.ret;
identifier f;
@@

f(...) { <+...
return 
-ret
+0
;@p
...+> }

@rewrite@
local idexpression r.ret;
expression e;
position p;
identifier change.f;
@@

f(...) { <+...
 \(ret@p = e\|&ret@p\)
...+> }

@depends on change@
local idexpression r.ret;
position p != rewrite.p;
identifier change.f;
@@

f(...) { <+...
(
break;
|
ret = 0;
... when exists
ret@p
... when any
|
- ret = 0;
)
...+> }

@depends on change@
identifier r.reti;
type T;
constant C;
identifier change.f;
@@

f(...) { ... when any
-T reti = C;
 ... when != reti
     when strict
 }

@depends on change@
identifier r.reti;
type T;
identifier change.f;
@@

f(...) { ... when any
-T reti;
 ... when != reti
     when strict
 }
// </smpl>

The first four rules detect cases where only 0 reaches a return.  The
remaining rules clean up any variable initializations or declarations that
are made unnecessary by this change.

[PATCH 6/13] [NETFILTER]: arp_tables: make return of 0 explicit

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Delete unnecessary local variable whose value is always 0 and that hides
the fact that the result is always 0.

A simplified version of the semantic patch that fixes this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
local idexpression ret;
expression e;
position p;
@@

-ret = 0;
... when != ret = e
return 
- ret
+ 0
  ;
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 net/ipv4/netfilter/arp_tables.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index f95b6f9..89f6737 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -1289,9 +1289,8 @@ compat_copy_entry_from_user(struct compat_arpt_entry *e, void **dstptr,
 	struct xt_target *target;
 	struct arpt_entry *de;
 	unsigned int origsize;
-	int ret, h;
+	int h;
 
-	ret = 0;
 	origsize = *size;
 	de = (struct arpt_entry *)*dstptr;
 	memcpy(de, e, sizeof(struct arpt_entry));
@@ -1312,7 +1311,7 @@ compat_copy_entry_from_user(struct compat_arpt_entry *e, void **dstptr,
 		if ((unsigned char *)de - base < newinfo->underflow[h])
 			newinfo->underflow[h] -= origsize - *size;
 	}
-	return ret;
+	return 0;
 }
 
 static int translate_compat_table(const char *name,

Re: [PATCH 6/13] [NETFILTER]: arp_tables: make return of 0 explicit

[Pablo Neira Ayuso]

Hi Julia,

On Mon, May 19, 2014 at 06:31:08AM +0200, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> Delete unnecessary local variable whose value is always 0 and that hides
> the fact that the result is always 0.
>
> A simplified version of the semantic patch that fixes this problem is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r exists@
> local idexpression ret;
> expression e;
> position p;
> @@
>
> -ret = 0;
> ... when != ret = e
> return
> - ret
> + 0
>   ;
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---
>  net/ipv4/netfilter/arp_tables.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> index f95b6f9..89f6737 100644
> --- a/net/ipv4/netfilter/arp_tables.c
> +++ b/net/ipv4/netfilter/arp_tables.c
> @@ -1289,9 +1289,8 @@ compat_copy_entry_from_user(struct compat_arpt_entry *e, void **dstptr,
>  	struct xt_target *target;
>  	struct arpt_entry *de;
>  	unsigned int origsize;
> -	int ret, h;
> +	int h;
>
> -	ret = 0;
>  	origsize = *size;
>  	de = (struct arpt_entry *)*dstptr;
>  	memcpy(de, e, sizeof(struct arpt_entry));
> @@ -1312,7 +1311,7 @@ compat_copy_entry_from_user(struct compat_arpt_entry *e, void **dstptr,
>  		if ((unsigned char *)de - base < newinfo->underflow[h])
>  			newinfo->underflow[h] -= origsize - *size;
>  	}
> -	return ret;
> +	return 0;
>  }

This looks good, but we can simplify this a bit further, given that
compat_copy_entry_from_user could be made void.

This branch below never happens.

        xt_entry_foreach(iter0, entry0, total_size) {
                ret = compat_copy_entry_from_user(iter0, &pos, &size,
                                                  name, newinfo, entry1);
                if (ret != 0)
                        break;
        }

Thanks for your patch anyway!

Re: [PATCH 6/13] [NETFILTER]: arp_tables: make return of 0 explicit

[Julia Lawall]

On Thu, 29 May 2014, Pablo Neira Ayuso wrote:

> Hi Julia,
> 
> On Mon, May 19, 2014 at 06:31:08AM +0200, Julia Lawall wrote:
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
> >
> > Delete unnecessary local variable whose value is always 0 and that hides
> > the fact that the result is always 0.
> >
> > A simplified version of the semantic patch that fixes this problem is as
> > follows: (http://coccinelle.lip6.fr/)
> >
> > // <smpl>
> > @r exists@
> > local idexpression ret;
> > expression e;
> > position p;
> > @@
> >
> > -ret = 0;
> > ... when != ret = e
> > return
> > - ret
> > + 0
> >   ;
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> >
> > ---
> >  net/ipv4/netfilter/arp_tables.c |    5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> > index f95b6f9..89f6737 100644
> > --- a/net/ipv4/netfilter/arp_tables.c
> > +++ b/net/ipv4/netfilter/arp_tables.c
> > @@ -1289,9 +1289,8 @@ compat_copy_entry_from_user(struct compat_arpt_entry *e, void **dstptr,
> >  	struct xt_target *target;
> >  	struct arpt_entry *de;
> >  	unsigned int origsize;
> > -	int ret, h;
> > +	int h;
> >
> > -	ret = 0;
> >  	origsize = *size;
> >  	de = (struct arpt_entry *)*dstptr;
> >  	memcpy(de, e, sizeof(struct arpt_entry));
> > @@ -1312,7 +1311,7 @@ compat_copy_entry_from_user(struct compat_arpt_entry *e, void **dstptr,
> >  		if ((unsigned char *)de - base < newinfo->underflow[h])
> >  			newinfo->underflow[h] -= origsize - *size;
> >  	}
> > -	return ret;
> > +	return 0;
> >  }
> 
> This looks good, but we can simplify this a bit further, given that
> compat_copy_entry_from_user could be made void.
> 
> This branch below never happens.
> 
>         xt_entry_foreach(iter0, entry0, total_size) {
>                 ret = compat_copy_entry_from_user(iter0, &pos, &size,
>                                                   name, newinfo, entry1);
>                 if (ret != 0)
>                         break;
>         }

Are you taking care of this., or should I do something?

thanks,
julia