Messages in this thread | | | Date | Tue, 13 May 2008 22:18:30 +0200 | From | Marcin Slusarz <> | Subject | Re: [PATCH] let ERR_PTR BUILD_BUG_ON when we know its argument is not a valid errno |
| |
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
| |