Re: [RFC] PTR_ERR: return 0 if ptr isn't an error value.

[Julia Lawall <julia.lawall@lip6.fr>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3142 bytes --]

On Mon, 3 Jun 2013, Uwe Kleine-König wrote:

> Hello Rusty,
> 
> [added akpm to Cc: who took the patch back then and Julia for the
> coccinelle part below]
> 
> On Mon, Jun 03, 2013 at 11:59:15AM +0930, Rusty Russell wrote:
> > 
> > Back in 2011, Uwe Kleine-König added the nonsensically-named
> > PTR_RET(), providing a means to avoid if() statements in code (commit 
> > fa9ee9c4b9).
> > 
> > Instead, just make PTR_ERR() return 0 if the pointer isn't an error
> > value.  This is harmless, since PTR_ERR() should have never been
> > passed a non-error value.  And GCC is usually smart enough to remove
> > the extra test if IS_ERR() has already been called.
> I wonder in which situations gcc fails to be smart enough. Did you check
> this?
> 
> > My vmlinux text increased by 300 bytes:
> > 
> > 	   text	   data	    bss	    dec	    hex	filename
> > 	6029452	 491628	2576384	9097464	 8ad0f8	vmlinux
> > 	6029721	 491628	2576384	9097733	 8ad205	vmlinux.PTR_ERR
> > 
> > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Cc: Thomas Meyer <thomas@m3y3r.de>
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > 
> > diff --git a/include/linux/err.h b/include/linux/err.h
> > index f2edce2..621d859 100644
> > --- a/include/linux/err.h
> > +++ b/include/linux/err.h
> > @@ -24,14 +24,16 @@ static inline void * __must_check ERR_PTR(long error)
> >  	return (void *) error;
> >  }
> >  
> > -static inline long __must_check PTR_ERR(const void *ptr)
> > +static inline long __must_check IS_ERR(const void *ptr)
> >  {
> > -	return (long) ptr;
> > +	return IS_ERR_VALUE((unsigned long)ptr);
> >  }
> >  
> > -static inline long __must_check IS_ERR(const void *ptr)
> > +static inline long __must_check PTR_ERR(const void *ptr)
> >  {
> > -	return IS_ERR_VALUE((unsigned long)ptr);
> > +	if (IS_ERR(ptr))
> > +		return (long) ptr;
> > +	return 0;
> >  }
> >  
> >  static inline long __must_check IS_ERR_OR_NULL(const void *ptr)
> > @@ -52,14 +54,7 @@ static inline void * __must_check ERR_CAST(const void *ptr)
> >  	return (void *) ptr;
> >  }
> >  
> > -static inline int __must_check PTR_RET(const void *ptr)
> > -{
> > -	if (IS_ERR(ptr))
> > -		return PTR_ERR(ptr);
> > -	else
> > -		return 0;
> > -}
> > -
> > +#define PTR_RET	PTR_ERR
> I'd add a comment here that PTR_RET shouldn't be used anymore.
> 
> >  #endif
> >  
> >  #endif /* _LINUX_ERR_H */
> > 
> 
> Is it worth to apply the same change to tools/virtio/linux/err.h to
> minimize the chance for later surprises?
> Also scripts/coccinelle/api/ptr_ret.cocci starts giving false warnings.
> 
> Other than that I think the change is fine.

For a random example, here is a function that currently uses PTR_RET:

static int __net_init iptable_raw_net_init(struct net *net)
{
        struct ipt_replace *repl;

        repl = ipt_alloc_initial_table(&packet_raw);
	if (repl == NULL)
                return -ENOMEM;
        net->ipv4.iptable_raw =
                ipt_register_table(net, &packet_raw, repl);
	kfree(repl);
        return PTR_RET(net->ipv4.iptable_raw);
}

If it becomes return PTR_ERR(...); at the end, won't it look like the 
function always fails?

julia