On Sat, Jun 21, 2014 at 01:59:15AM +0200, Thomas Gleixner wrote: > > > The problem is that we don't know if we should return here or break > > > here. If you don't understand the code, then it's best to just leave it > > > alone. > > Dan, what kind of attitude is that? > > Nick certainly found an issue where a possible NULL return from > affs_bread() can cause havoc. Yes. No arguments here. > Do YOU understand that code? > > If yes, you better explain, WHY Nicks finding is a false positive > instead of just telling him off in a very inpolite way. It's not a false positive at all. > If not, you better refrain from telling a reporter that he does not > understand the code and should stay away. Tone aside, he does have a point - namely, that patch in question doesn't contain any analysis of the bug and recovery strategy, turning a bug into something that is much harder to spot on inspection. If nothing else, such patches should contain a loud printk added on the b0rken codepath, along with a big fat warning in the source. I'm not saying that "fuck off unless you understand the code" is a sane policy - it's not. But "anything's better than an oops" is also wrong. > > > The problem is that we don't know if we should return here or break > > > here. > > The problem here is that proceeding with a known NULL pointer is wrong > to begin with. It does not matter at all whether break or return is > the proper thing to do. What matters is that proceeding with a NULL > pointer is wrong to begin with, no matter what. > > So either explain why this is a non issue and the NULL pointer return > cannot happen or shut up and try to find a proper solution for that > "return" vs. "break" issue. return vs. break here is the difference between discarding preallocated blocks and leaking them as well. The thing is, it's either severe OOM or a corrupted image. I'd *probably* go for "affs_error() and return" here, but that's not the only question - we probably ought to make it return an error, instead of having it void. And callers tend to do that affs_free_prealloc(), so much that I'm not sure if we actually want to keep it in affs_truncate() at all. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
