Re: [GIT PULL] scheduler fixes

[Linus Torvalds]

On Mon, 18 May 2009, Ingo Molnar wrote:
> 
> Rusty Russell (1):
>       sched: avoid flexible array member inside struct (gcc extension)

I'm not pulling this one either.

It makes no sense what-so-ever. It's uglier code, so calling it a cleanup 
is just wrong.

Now apart from being ugly and pointless, it is also FUNDAMENTALLY 
INCORRECT.

It is in no way true that "a union is the Right Way to do this", 
especially not the way THAT PIECE OF UTTER CRAP does it.

This is the original data structure:

	struct static_sched_group {
		struct sched_group sg;
		DECLARE_BITMAP(cpus, CONFIG_NR_CPUS);
	};

and it is fine (the fact that "sg" isn't fine is a totally different 
issue).

The new one:

	union static_sched_group {
		struct sched_group sg;
		char _sg_and_cpus[sizeof(struct sched_group) +
				  BITS_TO_LONGS(CONFIG_NR_CPUS) * sizeof(long)];
	};

claimed to be a "cleanup" (hah - what a f*cking joke! Anybody looking at 
it for half a second would see that that is a clear lie) is just a 
horrible bug waiting to happen.

You can't do that. Doing a character array with "sizeof" IS NOT VALID. It 
doesn't take things like different alignment into account. It might 
_work_, but it is still UTTER SH*T.

Yes, I'm upset. It's -rc6 and now two "please pull" requests have been 
totally unacceptable in very fundamental and obvious forms. 

I'm also upset becasue that obvious PIECE OF CRAP got two Acked-by's from 
people who should know better. 

If you wan tto fix this up, then just fix "struct sched_group sg" instead. 
Here's a suggested better fix, but I'd suggest somebody also write a 
honking big COMMENT in addition to this.

But note how simple this attached patch is? Note how it's not adding 
totally ugly and horrible code? THIS is a fix (and yes, zero-sized arrays 
are a gcc extensiontoo, but we've used them a lot)

I'm sure there are other ways to fix it too, and I'm open to them, but 
that union with character arrays etc I'm not open to.

			Linus
---
 include/linux/sched.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index b4c38bc..e0c9733 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -838,7 +838,7 @@ struct sched_group {
 	 */
 	u32 reciprocal_cpu_power;
 
-	unsigned long cpumask[];
+	unsigned long cpumask[0];
 };
 
 static inline struct cpumask *sched_group_cpus(struct sched_group *sg)
--
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/