Re: [2.6 patch] sound/oss/rme96xx.c: fix two check after use

[Al Viro]

On Wed, Apr 13, 2005 at 12:40:38PM +0200, Bodo Eggert <harvested.in.lkml@xxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> Al Viro <viro@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > On Wed, Apr 13, 2005 at 04:17:42AM +0200, Adrian Bunk wrote:
> 
> >> This patch fixes two check after use found by the Coverity checker.
> > 
> > Bullshit.  ->private_data is set by rme96xx_open() to guaranteed non-NULL
> > and never changed elsewhere.  Same comment about reading the fscking
> > source, BUG_ON(), etc.
> 
> If there are checks, they should be there for a purpose, and any sane
> reader will asume these checks to be nescensary.

Really?  Even in "obviously buggy code"?

> If they are dead code, you
> can say that, but please don't flame Adrian for fixing obviously buggy code

...

> in a way that is sane and at least more correct than the original without
> using several days of his lifetime to analyze the whole driver.

Funny, that.  "several days" in this case boils down to grep for accesses
to that field in driver (and stuff #included from it).  Which yields exactly
one assignment (in ->open()).  Combined with understanding that
	a) ->open() is definitely going to be executed before any calls of
->read() and
	b) nothing in generic code ever touches ->private_data
	c) if rme96xx_open() returns 0, it will leave us with non-NULL
->private_data.

Five minutes total.  And no, "fix" did not give more correct code -
in all cases it yields exactly the same behaviour.  All it does is
	* shifting what in effect is if (0) {do something odd} from
one place to another
	* making the warning go away

Note that warning had (correctly) pointed to fishy logics in the driver.
Shutting it up and leaving the real problem intact (and hidden) is
not particulary useful.

> Instead, you
> could provide the correct fix.

"Remove bogus checks".
-
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/