On Wed, 2012-04-04 at 16:00 -0700, Joe Perches wrote:
> Likely these should be && not ||
>
> drivers/scsi/FlashPoint.c: if(bit_cnt != 0 || bit_cnt != 8)
> drivers/media/dvb/dvb-usb/it913x.c: if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> drivers/media/dvb/dvb-usb/it913x.c: if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> drivers/media/rc/fintek-cir.c: if ((chip != 0x0408) || (chip != 0x0804))
It's been a month.
Do you maintainers not care
about these obvious defects?
On Fri, May 04, 2012 at 02:54:37PM -0700, Joe Perches wrote:
> On Wed, 2012-04-04 at 16:00 -0700, Joe Perches wrote:
> > drivers/media/dvb/dvb-usb/it913x.c: if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> > drivers/media/dvb/dvb-usb/it913x.c: if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> > drivers/media/rc/fintek-cir.c: if ((chip != 0x0408) || (chip != 0x0804))
> It's been a month.
> Do you maintainers not care
> about these obvious defects?
It'd probably help if you were to supply more analysis as to what the
issues you think you're seeing are - in the cases quoted above
(especially the last one) there's no obvious bug without further
analysis.
On Fri, 2012-05-04 at 23:02 +0100, Mark Brown wrote:
> On Fri, May 04, 2012 at 02:54:37PM -0700, Joe Perches wrote:
> > On Wed, 2012-04-04 at 16:00 -0700, Joe Perches wrote:
>
> > > drivers/media/dvb/dvb-usb/it913x.c: if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> > > drivers/media/dvb/dvb-usb/it913x.c: if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> > > drivers/media/rc/fintek-cir.c: if ((chip != 0x0408) || (chip != 0x0804))
>
> > It's been a month.
>
> > Do you maintainers not care
> > about these obvious defects?
>
> It'd probably help if you were to supply more analysis as to what the
> issues you think you're seeing are - in the cases quoted above
> (especially the last one) there's no obvious bug without further
> analysis.
Likely the || should be &&.
if (val != foo || test != bar)
is always true when foo != bar
On Fri, May 04, 2012 at 03:09:38PM -0700, Joe Perches wrote:
> On Fri, 2012-05-04 at 23:02 +0100, Mark Brown wrote:
> > On Fri, May 04, 2012 at 02:54:37PM -0700, Joe Perches wrote:
> > > > drivers/media/rc/fintek-cir.c: if ((chip != 0x0408) || (chip != 0x0804))
> > It'd probably help if you were to supply more analysis as to what the
> > issues you think you're seeing are - in the cases quoted above
> > (especially the last one) there's no obvious bug without further
> > analysis.
> Likely the || should be &&.
> if (val != foo || test != bar)
> is always true when foo != bar
Right, but you need to look at the code and explain why this is a
problem. For example, the case I've left quoted above reads to me like
the intention is "If the chip isn't one I know doesn't like this then
let's do it" which is a perfectly sensible thing to write.
On Fri, 04 May 2012 14:54:37 -0700
Joe Perches <[email protected]> wrote:
> On Wed, 2012-04-04 at 16:00 -0700, Joe Perches wrote:
> > Likely these should be && not ||
> >
> > drivers/scsi/FlashPoint.c: if(bit_cnt != 0 || bit_cnt != 8)
>
> It's been a month.
>
> Do you maintainers not care about these obvious defects?
You are assuming the code in question has maintainers. FlashPoint hasn't
really had a maintainer since Leonard died. Also given it's been like
that for approaching twenty years and works there is a real danger in
changing it unless the patch is tested carefully - if you can find anyone
with the hardware.
It's not unknown for code to work *because* it is wrong.
Alan
On Fri, 2012-05-04 at 23:56 +0100, Alan Cox wrote:
> On Fri, 04 May 2012 14:54:37 -0700
> Joe Perches <[email protected]> wrote:
>
> > On Wed, 2012-04-04 at 16:00 -0700, Joe Perches wrote:
> > > Likely these should be && not ||
> > >
> > > drivers/scsi/FlashPoint.c: if(bit_cnt != 0 || bit_cnt != 8)
> >
> > It's been a month.
> >
> > Do you maintainers not care about these obvious defects?
>
> You are assuming the code in question has maintainers.
Nope. I'm assuming the nominal subsystem maintainers
haven't bothered to look at it or are in "don't care"
mode and I am simply prompting people to look again.
Richard Weinberger replied showing that the flashpoint
bit was commented out. Thanks Richard.
cheers, Joe
On Fri, May 04, 2012 at 06:13:33PM -0400, Mark Brown wrote:
> On Fri, May 04, 2012 at 03:09:38PM -0700, Joe Perches wrote:
> > On Fri, 2012-05-04 at 23:02 +0100, Mark Brown wrote:
> > > On Fri, May 04, 2012 at 02:54:37PM -0700, Joe Perches wrote:
>
> > > > > drivers/media/rc/fintek-cir.c: if ((chip != 0x0408) || (chip != 0x0804))
>
> > > It'd probably help if you were to supply more analysis as to what the
> > > issues you think you're seeing are - in the cases quoted above
> > > (especially the last one) there's no obvious bug without further
> > > analysis.
>
> > Likely the || should be &&.
>
> > if (val != foo || test != bar)
>
Guess that was meant to be
if (val != foo || val != bar)
> > is always true when foo != bar
>
> Right, but you need to look at the code and explain why this is a
> problem. For example, the case I've left quoted above reads to me like
> the intention is "If the chip isn't one I know doesn't like this then
> let's do it" which is a perfectly sensible thing to write.
I can not really follow your logic here; it is difficult for me to imagine a situation
where anything along the line of
if (val != 1 || val != 2)
would provide value other than creating confusion. Maybe you can explain that a bit further.
Commit 83ec8225b6 ([media] fintek-cir: add support for newer chip version) suggests
that the code was added to support new chip revisions in addition to old revisions.
What it does instead is to drop support for old revisions. To accomplish that, it would
have been sufficient to replace the define of LOGICAL_DEV_CIR, ie change it from 0x05
to 0x08. That would have been a one-line patch, much simpler than the patch as it was
applied. If the idea was to drop support for old revisions in favor of new ones while
keeping a reference to the old definition, it would have been sufficient to add a comment
next to the definition of LOGICAL_DEV_CIR, without all the code complexity introduced
by the patch.
Maybe the author of the above patch might want to comment.
Thanks,
Guenter
On Sat, May 05, 2012 at 09:00:42AM -0700, Guenter Roeck wrote:
> On Fri, May 04, 2012 at 06:13:33PM -0400, Mark Brown wrote:
> > Right, but you need to look at the code and explain why this is a
> > problem. For example, the case I've left quoted above reads to me like
> > the intention is "If the chip isn't one I know doesn't like this then
> > let's do it" which is a perfectly sensible thing to write.
> I can not really follow your logic here; it is difficult for me to imagine a situation
> where anything along the line of
> if (val != 1 || val != 2)
> would provide value other than creating confusion. Maybe you can explain that a bit further.
Yeah, I hadn't actually read the code closely enough but it's not my
main point anyway - the main point was that the reports were very easy
to ignore because they're just a paste in of the error message with no
analysis they were very likely to just get ignored unless someone has a
particular interest in the code (which is essentially what I did - I
glanced at the report but only very briefly).
Compare this with the reports from people like Julia Lawall, for example
- they tend to be very clear. Even simply adding "...as with || they
will always be true" would've helped.
On Mon, 2012-05-07 at 09:36 +0100, Mark Brown wrote:
> On Sat, May 05, 2012 at 09:00:42AM -0700, Guenter Roeck wrote:
> > On Fri, May 04, 2012 at 06:13:33PM -0400, Mark Brown wrote:
>
> > > Right, but you need to look at the code and explain why this is a
> > > problem. For example, the case I've left quoted above reads to me like
> > > the intention is "If the chip isn't one I know doesn't like this then
> > > let's do it" which is a perfectly sensible thing to write.
>
> > I can not really follow your logic here; it is difficult for me to imagine a situation
> > where anything along the line of
> > if (val != 1 || val != 2)
> > would provide value other than creating confusion. Maybe you can explain that a bit further.
>
> Yeah, I hadn't actually read the code closely enough but it's not my
> main point anyway - the main point was that the reports were very easy
> to ignore because they're just a paste in of the error message
Nope, these were the original source codes.
> with no analysis
You did elide the "Likely the || should be &&" preface.
> they were very likely to just get ignored unless someone has a
> particular interest in the code (which is essentially what I did - I
> glanced at the report but only very briefly).
No worries, I miss things when I scan code too quickly
as well.
> Compare this with the reports from people like Julia Lawall, for example
> - they tend to be very clear. Even simply adding "...as with || they
> will always be true" would've helped.
I think it's a pretty basic logic error that most all
lkml readers should be able to identify most of the time.
cheers, Joe
On Mon, May 07, 2012 at 09:20:34AM -0700, Joe Perches wrote:
> On Mon, 2012-05-07 at 09:36 +0100, Mark Brown wrote:
> > with no analysis
> You did elide the "Likely the || should be &&" preface.
By analysis I meant something like "...because X".
> > Compare this with the reports from people like Julia Lawall, for example
> > - they tend to be very clear. Even simply adding "...as with || they
> > will always be true" would've helped.
> I think it's a pretty basic logic error that most all
> lkml readers should be able to identify most of the time.
"Can spot" and "will spot" are two different things; speaking as someone
on the receiving end of lots of mails it does make a big difference.
Cues in the human written text which show someone thought about the
problem help a lot with catching attention - the more effort it takes to
understand a mail the more likely it is to get skipped, especially if
there's no patch.