This patch fixes two check after use found by the Coverity checker.
Signed-off-by: Adrian Bunk <[email protected]>
---
This patch was already sent on:
- 27 Mar 2005
--- linux-2.6.12-rc1-mm3-full/sound/oss/rme96xx.c.old 2005-03-27 23:16:02.000000000 +0200
+++ linux-2.6.12-rc1-mm3-full/sound/oss/rme96xx.c 2005-03-27 23:16:11.000000000 +0200
@@ -1534,18 +1534,20 @@
static ssize_t rme96xx_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos)
{
struct dmabuf *dma = (struct dmabuf *)file->private_data;
ssize_t ret = 0;
int cnt; /* number of bytes from "buffer" that will/can be used */
- int hop = count/dma->outchannels;
+ int hop;
int hwp;
int exact = (file->f_flags & O_NONBLOCK);
if(dma == NULL || (dma->s) == NULL)
return -ENXIO;
+ hop = count/dma->outchannels;
+
if (dma->mmapped || !dma->opened)
return -ENXIO;
if (!access_ok(VERIFY_READ, buffer, count))
return -EFAULT;
@@ -1599,18 +1601,20 @@
static ssize_t rme96xx_read(struct file *file, char __user *buffer, size_t count, loff_t *ppos)
{
struct dmabuf *dma = (struct dmabuf *)file->private_data;
ssize_t ret = 0;
int cnt; /* number of bytes from "buffer" that will/can be used */
- int hop = count/dma->inchannels;
+ int hop;
int hwp;
int exact = (file->f_flags & O_NONBLOCK);
if(dma == NULL || (dma->s) == NULL)
return -ENXIO;
+ hop = count/dma->inchannels;
+
if (dma->mmapped || !dma->opened)
return -ENXIO;
if (!access_ok(VERIFY_WRITE, buffer, count))
return -EFAULT;
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.
Al Viro <[email protected]> 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. 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. Instead, you
could provide the correct fix.
--
Funny quotes:
33. If lawyers are disbarred and clergymen defrocked, doesn't it follow that
electricians can be delighted, musicians denoted, cowboys deranged, models
deposed, tree surgeons debarked, and dry cleaners depressed?
On Wed, Apr 13, 2005 at 12:40:38PM +0200, Bodo Eggert <[email protected]> wrote:
> If there are checks, they should be there for a purpose,
emphasis here is on _should_
> and any sane reader will asume these checks to be nescensary.
That's a bad assumptions when you're deadling with drivers or software of
similar quality.
> 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. Instead, you
> could provide the correct fix.
The correct fix is to remove the check. And no, we don't have a rule that
someone must provide something better when trying to critize it.
On Wed, Apr 13, 2005 at 12:40:38PM +0200, Bodo Eggert <[email protected]> wrote:
> Al Viro <[email protected]> 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".