[PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses around operand of ‘!’

[PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses around operand of ‘!’

[Sergey Senozhatsky]

Trivial: fixing gcc 4.4 compiler warning:
drivers/scsi/st.c: In function ‘st_int_ioctl’:
drivers/scsi/st.c:2967: warning: suggest parentheses around operand of ‘!’ or change ‘|’ to ‘||’ or ‘!’ to ‘~’

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>

---

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index eb24efe..2bde8a5 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -2964,7 +2964,7 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
 			    !(STp->use_pf & PF_TESTED)) {
 				/* Try the other possible state of Page Format if not
 				   already tried */
-				STp->use_pf = !STp->use_pf | PF_TESTED;
+				STp->use_pf = (!STp->use_pf) | PF_TESTED;
 				st_release_request(SRpnt);
 				SRpnt = NULL;
 				return st_int_ioctl(STp, cmd_in, arg);

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

Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses around operand of ‘!’

[James Bottomley]

On Thu, 2009-04-30 at 09:04 +0300, Sergey Senozhatsky wrote:
> Trivial: fixing gcc 4.4 compiler warning:
> drivers/scsi/st.c: In function ‘st_int_ioctl’:
> drivers/scsi/st.c:2967: warning: suggest parentheses around operand of ‘!’ or change ‘|’ to ‘||’ or ‘!’ to ‘~’
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> 
> ---
> 
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index eb24efe..2bde8a5 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -2964,7 +2964,7 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
>  			    !(STp->use_pf & PF_TESTED)) {
>  				/* Try the other possible state of Page Format if not
>  				   already tried */
> -				STp->use_pf = !STp->use_pf | PF_TESTED;
> +				STp->use_pf = (!STp->use_pf) | PF_TESTED;

This just doesn't look right.  As I said to the warning reporter, I
think STp->use_pf |= PF_TESTED; is the correct fix, but I need Kai to
check this.

James


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

Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses around operand of ‘!’

[Andrew Morton]

On Thu, 30 Apr 2009 21:31:24 +0000
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Thu, 2009-04-30 at 09:04 +0300, Sergey Senozhatsky wrote:
> > Trivial: fixing gcc 4.4 compiler warning:
> > drivers/scsi/st.c: In function ‘st_int_ioctl’:
> > drivers/scsi/st.c:2967: warning: suggest parentheses around operand of ‘!’ or change ‘|’ to ‘||’ or ‘!’ to ‘~’
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> > 
> > ---
> > 
> > diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> > index eb24efe..2bde8a5 100644
> > --- a/drivers/scsi/st.c
> > +++ b/drivers/scsi/st.c
> > @@ -2964,7 +2964,7 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
> >  			    !(STp->use_pf & PF_TESTED)) {
> >  				/* Try the other possible state of Page Format if not
> >  				   already tried */
> > -				STp->use_pf = !STp->use_pf | PF_TESTED;
> > +				STp->use_pf = (!STp->use_pf) | PF_TESTED;
> 
> This just doesn't look right.  As I said to the warning reporter, I
> think STp->use_pf |= PF_TESTED; is the correct fix, but I need Kai to
> check this.

yeah, good point.  The code looks very confused.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses around operand of ‘!’

[Sergey Senozhatsky]

On (30/04/09 21:31), James Bottomley wrote:
> To: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> Cc: Andrew Morton <akpm@linux-foundation.org>, linux-scsi@vger.kernel.org,
> 	Kai Makisara <Kai.Makisara@kolumbus.fi>
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> Subject: Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses
> 	around operand of ‘!’
> 
> On Thu, 2009-04-30 at 09:04 +0300, Sergey Senozhatsky wrote:
> > Trivial: fixing gcc 4.4 compiler warning:
> > drivers/scsi/st.c: In function ‘st_int_ioctl’:
> > drivers/scsi/st.c:2967: warning: suggest parentheses around operand of ‘!’ or change ‘|’ to ‘||’ or ‘!’ to ‘~’
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> > 
> > ---
> > 
> > diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> > index eb24efe..2bde8a5 100644
> > --- a/drivers/scsi/st.c
> > +++ b/drivers/scsi/st.c
> > @@ -2964,7 +2964,7 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
> >  			    !(STp->use_pf & PF_TESTED)) {
> >  				/* Try the other possible state of Page Format if not
> >  				   already tried */
> > -				STp->use_pf = !STp->use_pf | PF_TESTED;
> > +				STp->use_pf = (!STp->use_pf) | PF_TESTED;
> 

> This just doesn't look right.  As I said to the warning reporter, I
> think STp->use_pf |= PF_TESTED; is the correct fix, but I need Kai to
> check this.
> 
> James
> 

Hello James.
It's not clear to me why STp->use_pf |= PF_TESTED; is correct fix 
(since STp->use_pf = (!STp->use_pf) | PF_TESTED; is not equals to STp->use_pf |= PF_TESTED;).

Would you provide some explanations?

Thanks.

Sergey.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses around operand of ‘!’

[James Bottomley]

On Fri, 2009-05-01 at 00:59 +0300, Sergey Senozhatsky wrote:
> On (30/04/09 21:31), James Bottomley wrote:
> > To: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> > Cc: Andrew Morton <akpm@linux-foundation.org>, linux-scsi@vger.kernel.org,
> > 	Kai Makisara <Kai.Makisara@kolumbus.fi>
> > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Subject: Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses
> > 	around operand of ‘!’
> > 
> > On Thu, 2009-04-30 at 09:04 +0300, Sergey Senozhatsky wrote:
> > > Trivial: fixing gcc 4.4 compiler warning:
> > > drivers/scsi/st.c: In function ‘st_int_ioctl’:
> > > drivers/scsi/st.c:2967: warning: suggest parentheses around operand of ‘!’ or change ‘|’ to ‘||’ or ‘!’ to ‘~’
> > > 
> > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> > > 
> > > ---
> > > 
> > > diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> > > index eb24efe..2bde8a5 100644
> > > --- a/drivers/scsi/st.c
> > > +++ b/drivers/scsi/st.c
> > > @@ -2964,7 +2964,7 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
> > >  			    !(STp->use_pf & PF_TESTED)) {
> > >  				/* Try the other possible state of Page Format if not
> > >  				   already tried */
> > > -				STp->use_pf = !STp->use_pf | PF_TESTED;
> > > +				STp->use_pf = (!STp->use_pf) | PF_TESTED;
> > 
> 
> > This just doesn't look right.  As I said to the warning reporter, I
> > think STp->use_pf |= PF_TESTED; is the correct fix, but I need Kai to
> > check this.
> > 
> > James
> > 
> 
> Hello James.
> It's not clear to me why STp->use_pf |= PF_TESTED; is correct fix 
> (since STp->use_pf = (!STp->use_pf) | PF_TESTED; is not equals to STp->use_pf |= PF_TESTED;).
> 
> Would you provide some explanations?

Well, I know why (!STp->use_pf) is wrong: it would set USE_PF in an
incredibly obscure fashon.  The flag PF_TESTED seems to imply that it's
a don't check this again type deal, and the if clause is looking for it
not being set, hence I think the most likely thing to be done here is
set the PF_TESTED flag.

James


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

Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses around operand of ???!???

[Al Viro]

On Thu, Apr 30, 2009 at 10:08:44PM +0000, James Bottomley wrote:

> Well, I know why (!STp->use_pf) is wrong: it would set USE_PF in an
> incredibly obscure fashon.  The flag PF_TESTED seems to imply that it's
> a don't check this again type deal, and the if clause is looking for it
> not being set, hence I think the most likely thing to be done here is
> set the PF_TESTED flag.

Not _set_ USE_PF; what's happening there is
	^= USE_PF | PF_TESTED; 	/* we know that PF_TESTED was not set */

Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses around operand of ???!???

[Sergey Senozhatsky]

On (30/04/09 23:26), Al Viro wrote:
> On Thu, Apr 30, 2009 at 10:08:44PM +0000, James Bottomley wrote:
> 
> > Well, I know why (!STp->use_pf) is wrong: it would set USE_PF in an
> > incredibly obscure fashon.  The flag PF_TESTED seems to imply that it's
> > a don't check this again type deal, and the if clause is looking for it
> > not being set, hence I think the most likely thing to be done here is
> > set the PF_TESTED flag.
> 
> Not _set_ USE_PF; what's happening there is
> 	^= USE_PF | PF_TESTED; 	/* we know that PF_TESTED was not set */
> 

st.h:#define PF_TESTED   2
st.h:#define USE_PF      1

st.c:2963
if (cmdstatp->sense_hdr.sense_key == ILLEGAL_REQUEST && !(STp->use_pf & PF_TESTED))

'&& !(STp->use_pf & PF_TESTED)' means: STp->use_pf == 0 or STp->use_pf != ... 0010 (PF_TESTED).

if == 0
STp->use_pf = !0 | 2    -> 3

if > PF_TESTED
STp->use_pf = 0 | PF_TESTED     -> PF_TESTED

I need to think about
> I think the most likely thing to be done here is set the PF_TESTED flag.

Sergey.

Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses around operand of ???!???

[Kai Makisara]

On Thu, 30 Apr 2009, Al Viro wrote:

> On Thu, Apr 30, 2009 at 10:08:44PM +0000, James Bottomley wrote:
> 
> > Well, I know why (!STp->use_pf) is wrong: it would set USE_PF in an
> > incredibly obscure fashon.  The flag PF_TESTED seems to imply that it's
> > a don't check this again type deal, and the if clause is looking for it
> > not being set, hence I think the most likely thing to be done here is
> > set the PF_TESTED flag.
> 
> Not _set_ USE_PF; what's happening there is
> 	^= USE_PF | PF_TESTED; 	/* we know that PF_TESTED was not set */
> 
Yes, this is the correct interpretation. This "bug" has been on my list of 
things to fix since David Binderman reported it to me earlier. The fix has 
waited because the current code is correct but difficult to read. The fix 
I have been planning is

        STp->use_pf = (STp->use_pf ^ USE_PF) | PF_TESTED;

This is a little more explicit in showing that we want to flip the bit 
masked by USE_PF and tell that we have tried both alternatives.

I can make a patch later today or tomorrow.

Thanks,
Kai

[PATCH] SCSI tape: fix gcc 4.4 warning

[Kai Makisara]

This patch fixes the GCC 4.4 warning reported by David Binderman and Sergey
Senozhatsky. The old version was working correctly but was not easy to read.

Signed-off-by: Kai Makisara <kai.makisara@kolumbus.fi>
---
 drivers/scsi/st.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index eb24efe..c3334ef 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -2964,7 +2964,7 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
 			    !(STp->use_pf & PF_TESTED)) {
 				/* Try the other possible state of Page Format if not
 				   already tried */
-				STp->use_pf = !STp->use_pf | PF_TESTED;
+				STp->use_pf = (STp->use_pf ^ USE_PF) | PF_TESTED;
 				st_release_request(SRpnt);
 				SRpnt = NULL;
 				return st_int_ioctl(STp, cmd_in, arg);