Re: [PATCH] let ERR_PTR BUILD_BUG_ON when we know its argument is not a valid errno

[Marcin Slusarz <marcin.slusarz@gmail.com>]

On Mon, May 12, 2008 at 04:38:30PM -0700, Andrew Morton wrote:
> On Sun, 11 May 2008 22:12:14 +0200
> Marcin Slusarz <marcin.slusarz@gmail.com> wrote:
> 
> > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > Cc: Andrew Morton <akpm@osdl.org>
> > ---
> > allmodconfig compile tested (on x86_64)
> > 
> > should be applied after:
> > net/sunrpc/xprtrdma: fix svc_rdma_create out of memory error path
> > jfs: 0 is not valid errno value
> > ---
> >  include/linux/err.h |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/linux/err.h b/include/linux/err.h
> > --- a/include/linux/err.h
> > +++ b/include/linux/err.h
> > @@ -19,11 +19,13 @@
> >  
> >  #define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> >  
> > -static inline void *ERR_PTR(long error)
> > +static inline void *__ERR_PTR(long error)
> >  {
> >  	return (void *) error;
> >  }
> >  
> > +#define ERR_PTR(error) (BUILD_BUG_ON(!IS_ERR_VALUE(error)), __ERR_PTR(error))
> > +
> >  static inline long PTR_ERR(const void *ptr)
> >  {
> >  	return (long) ptr;
> 
> Not sure about this one.  BUILD_BUG_ON only makes sense if the value is
> a compile-time constant.  I think the code as you have it will take this:
> 
> 	int e = foo();
> 
> 	p = ERR_PTR(e);
> 
> and will attempt to evaluate sizeof() on a negative-sized array at
> runtime.  The conmpile will laugh and throw that all away, but it's a
> bit weird.
> 
> Plus I'd have thought that the amount of code which does ERR_PTR(-EFOO)
> is fairly small, but perhaps that's wrong.

$ git grep 'ERR_PTR(-E[A-Z]*)' | wc -l
1431

> If I _am_ wrong then I do think it'd be saner to only do the
> BUILD_BUG_ON() if __builtin_constant_p(error) evaluates true.  And even

I thought BUILD_BUG_ON uses __builtin_constant_p internally and it was
a big mistake (see below).

> then I do think we'd like to see a more lengthy justification of why
> the kernel needs this check.

Well, I think it's better to find more errors at compile time,
than on rare runtime situation (error handling).
This patch found 2 errors on current sources (but one of them was harmless).

>  More lengthy than zero, anyway...
> 
> (If a compile-time check is needed then why not a runtime one also?)
I'm not sure - it would make kernel slightly bigger. I'll check that.

Today I discovered, that this patch causes funny runtime errors (/proc is mounted,
but many applications think it's not), so ignore this patch for now.
I'll prepare second version.

Marcin