lkml.org 
[lkml]   [2007]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [git patches] two warning fixes
Date
On Jul 19, 2007, at 14:04:29, Linus Torvalds wrote:
> On Thu, 19 Jul 2007, Krzysztof Halasa wrote:
>> Jeff Garzik <jeff@garzik.org> writes:
>>> My overall goal is killing useless warnings that continually
>>> obscure real ones.
>>
>> Precisely, the goal should be to make must_check (and similar
>> things) warn only in real cases.
>
> .. the problem with that mentality is that it's not how people work.
>
> People shut up warnings by adding code.
>
> Adding code tends to add bugs.
>
> People don't generally think "maybe that warning was bogus".
>
> More people *should* generally ask themselves: "was the warning
> worth it?" and then, if the answer is "no", they shouldn't add
> code, they should remove the thing that causes the warning in the
> first place.
>
> For example, for compiler options, the correct thign is often to
> just say "that option was broken", and not use "-fsign-warning",
> for example. We've literally have had bugs *added* because people
> "fixed" a sign warning. More than once, in fact.
>
> Every time you see a warning, you should ask yourself: is the
> warning interesting, correct and valid? And if it isn't all three,
> then the problem is whatever *causes* the warning, not the code
> itself.

I agree that there are a fair number of things (like the sysfs calls)
that should just WARN() when they hit an error, but I also think that
we're currently missing a *lot* of __must_check's that we should
have. For example a friend of mine was having problems with an HDAPS
patch where it just kind of hung. Turns out the problem was that the
code blithely called scsi_execute_async() and then put itself to
sleep on a completion... except scsi_execute_async() returned failure
and the completion would never complete.

For instance, I would bet that a fair number of the other int-
returning functions in include/scsi/scsi_device.h want __must_check
on them. That said, the person adding the __must_check should be
REQUIRED to do at least a superficial audit of the code.

I'd propose a few simple rules:
(1) If it can return the only pointer to freshly-allocated pointer
then it's __must_check
(2) If it can return a hard error which the caller must handle
specially, then it's __must_check
(3) If the only possible error is a kernel bug then make the damn
thing return void and give it a big fat WARN() when it fails.
(4) For any other case (or if you are unsure), don't flag it.

And of course the burden of proof is on the person trying to add the
__must_check.

Cheers,
Kyle Moffett

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

\
 
 \ /
  Last update: 2007-07-23 05:29    [from the cache]
©2003-2011 Jasper Spaans