Re: random.c changes for sparse irq_desc are crap

[Matt Mackall]

On Wed, 2008-12-31 at 16:14 -0800, Yinghai Lu wrote:
> On Wed, Dec 31, 2008 at 3:40 PM, Matt Mackall <mpm@xxxxxxxxxxx> wrote:
> > On Wed, 2008-12-31 at 15:07 -0800, Yinghai Lu wrote:
> >> Matt Mackall wrote:
> >> > I just noticed you merged a change that pointlessly converts two
> >> > random.c functions into ugly random.h inlines without going through the
> >> > maintainer.
> >> >
> >> > I also don't like the look of the newly-introduced sparse variants of
> >> > these functions. Failure to find an irq descriptor in
> >> > get_timer_rand_state is a BUG_ON should-never-happen sort of condition,
> >> > not something to silently ignore. Letting the code try to dereference
> >> > NULL is preferred here: we'll actually be able to find and fix the
> >> > broken driver that's throwing around meaningless irq vectors.
> >> >
> >> > Throwing away the timer_state pointer in the set_timer_rand_state
> >> > function is similarly bogus in addition to being a memory leak.
> >> >
> >> > Please fix this up.
> >> >
> >>
> >> want something like this?
> >
> > Not quite.
> >
> > First, please turn these back into normal functions in random.c.
> > Inlining functions is generally discouraged these days unless you have a
> > good reason and numbers to back it up.
> 
> Ingo wanted to hide that #ifdef to .h

Ahh. I think that makes sense in some situations, but I'd prefer not to
do that here. Some headers are actually meant to be more readable than
the corresponding .c file. Also, we still end up with an ifdef in the .c
file so we're not actually winning.

-- 
Mathematics is the supreme nostalgia of our time.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/