Messages in this thread | | | Date | Mon, 12 Jan 2009 11:54:38 -0800 | From | "Pallipadi, Venkatesh" <> | Subject | Re: [git pull] x86 fixes |
| |
On Mon, Jan 12, 2009 at 11:47:13AM -0800, Linus Torvalds wrote: > > > On Mon, 12 Jan 2009, Pallipadi, Venkatesh wrote: > > + if (strict_prot || > > + (want_flags == _PAGE_CACHE_UC_MINUS && > > + flags == _PAGE_CACHE_WB) || > > + (want_flags == _PAGE_CACHE_WC && > > + flags == _PAGE_CACHE_WB)) { > > Please don't write code like this. > > Do it as an inline function that returns true/false and has comments on > what the hell is going on. > > If a conditional doesn't fit on one line, it should generally be > abstracted away into a readable function where the name explains what it > does conceptually. >
Yes. The actual patch that is lined up in tip fixes indeed has this as a macro sharing this code with 2 callers and comment about this (is_new_memtype_allowed()). I wanted to keep the changes smaller in this test patch, which is just to root cause this particular crash and ended up with above code.
Thanks, Venki
| |