[PATCH 0/1] delete unnecessary null test on array

[PATCH 0/1] delete unnecessary null test on array

[Julia Lawall]

Delete NULL test on array.  The complete semantic patch that finds this
problem is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r@
type T;
T [] e;
position p;
@@

(
 e ==@p NULL
|
 e !=@p NULL
|
 !@p e
)

@ disable fld_to_ptr@
expression e;
identifier f;
position r.p;
@@

(
* (e.f) ==@p NULL
|
* (e.f) !=@p NULL
|
* !@p(e.f)
)
// </smpl>

For best results, this semantic patch requires lots of type information,
and should be used with the options --recursive-includes and
--relax-include-path.  This may take a long time to run.

[PATCH 1/1] dpt_i2o: delete unnecessary null test on array

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Delete NULL test on array (always false).

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r@
type T;
T [] e;
position p;
@@
e ==@p NULL

@ disable fld_to_ptr@
expression e;
identifier f;
position r.p;
@@
* e.f ==@p NULL
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
I don't know if this is the correct change, or if some other test was
intended.  But the code has been this way since at least 2.4.20, so it
would seem that no one has been bothered by the lack of whatever this was
supposed to test for.

 drivers/scsi/dpt_i2o.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index 67283ef..62e276b 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -1169,11 +1169,6 @@ static struct adpt_device* adpt_find_device(adpt_hba* pHba, u32 chan, u32 id, u6
 	if(chan < 0 || chan >= MAX_CHANNEL)
 		return NULL;
 	
-	if( pHba->channel[chan].device == NULL){
-		printk(KERN_DEBUG"Adaptec I2O RAID: Trying to find device before they are allocated\n");
-		return NULL;
-	}
-
 	d = pHba->channel[chan].device[id];
 	if(!d || d->tid == 0) {
 		return NULL;

Re: [PATCH 1/1] dpt_i2o: delete unnecessary null test on array

[walter harms]

Am 06.08.2014 12:39, schrieb Julia Lawall:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Delete NULL test on array (always false).
> 
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r@
> type T;
> T [] e;
> position p;
> @@
> e ==@p NULL
> 
> @ disable fld_to_ptr@
> expression e;
> identifier f;
> position r.p;
> @@
> * e.f ==@p NULL
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
> I don't know if this is the correct change, or if some other test was
> intended.  But the code has been this way since at least 2.4.20, so it
> would seem that no one has been bothered by the lack of whatever this was
> supposed to test for.
> 
>  drivers/scsi/dpt_i2o.c |    5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
> index 67283ef..62e276b 100644
> --- a/drivers/scsi/dpt_i2o.c
> +++ b/drivers/scsi/dpt_i2o.c
> @@ -1169,11 +1169,6 @@ static struct adpt_device* adpt_find_device(adpt_hba* pHba, u32 chan, u32 id, u6
>  	if(chan < 0 || chan >= MAX_CHANNEL)
>  		return NULL;
>  	

chan is u32 and u32 < 0 ?
for the next round.

re,
 wh

> -	if( pHba->channel[chan].device == NULL){
> -		printk(KERN_DEBUG"Adaptec I2O RAID: Trying to find device before they are allocated\n");
> -		return NULL;
> -	}
> -
>  	d = pHba->channel[chan].device[id];
>  	if(!d || d->tid == 0) {
>  		return NULL;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/1] dpt_i2o: delete unnecessary null test on array

[James Bottomley]

On Wed, 2014-08-06 at 12:39 +0200, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Delete NULL test on array (always false).
> 
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r@
> type T;
> T [] e;
> position p;
> @@
> e ==@p NULL
> 
> @ disable fld_to_ptr@
> expression e;
> identifier f;
> position r.p;
> @@
> * e.f ==@p NULL
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
> I don't know if this is the correct change, or if some other test was
> intended.  But the code has been this way since at least 2.4.20, so it
> would seem that no one has been bothered by the lack of whatever this was
> supposed to test for.
> 
>  drivers/scsi/dpt_i2o.c |    5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
> index 67283ef..62e276b 100644
> --- a/drivers/scsi/dpt_i2o.c
> +++ b/drivers/scsi/dpt_i2o.c
> @@ -1169,11 +1169,6 @@ static struct adpt_device* adpt_find_device(adpt_hba* pHba, u32 chan, u32 id, u6
>  	if(chan < 0 || chan >= MAX_CHANNEL)
>  		return NULL;
>  	
> -	if( pHba->channel[chan].device == NULL){
> -		printk(KERN_DEBUG"Adaptec I2O RAID: Trying to find device before they are allocated\n");
> -		return NULL;
> -	}

dpt_i2o is always weirdly fun, but I think, based on the message, this
check is supposed to be 

if( pHba->channel[chan].device[id] == NULL){

Since device is an array of device pointers which are allocated by
parsing data.

James

Re: [PATCH 1/1] dpt_i2o: delete unnecessary null test on array

[Julia Lawall]

> > diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
> > index 67283ef..62e276b 100644
> > --- a/drivers/scsi/dpt_i2o.c
> > +++ b/drivers/scsi/dpt_i2o.c
> > @@ -1169,11 +1169,6 @@ static struct adpt_device* adpt_find_device(adpt_hba* pHba, u32 chan, u32 id, u6
> >  	if(chan < 0 || chan >= MAX_CHANNEL)
> >  		return NULL;
> >
> > -	if( pHba->channel[chan].device == NULL){
> > -		printk(KERN_DEBUG"Adaptec I2O RAID: Trying to find device before they are allocated\n");
> > -		return NULL;
> > -	}
>
> dpt_i2o is always weirdly fun, but I think, based on the message, this
> check is supposed to be
>
> if( pHba->channel[chan].device[id] == NULL){
>
> Since device is an array of device pointers which are allocated by
> parsing data.

That seems to be already checked immediately below:

        d = pHba->channel[chan].device[id];
	if(!d || d->tid == 0) {
                return NULL;
        }

julia

Re: [PATCH 1/1] dpt_i2o: delete unnecessary null test on array

[James Bottomley]

On Fri, 2014-08-08 at 19:03 +0200, Julia Lawall wrote:
> > > diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
> > > index 67283ef..62e276b 100644
> > > --- a/drivers/scsi/dpt_i2o.c
> > > +++ b/drivers/scsi/dpt_i2o.c
> > > @@ -1169,11 +1169,6 @@ static struct adpt_device* adpt_find_device(adpt_hba* pHba, u32 chan, u32 id, u6
> > >  	if(chan < 0 || chan >= MAX_CHANNEL)
> > >  		return NULL;
> > >
> > > -	if( pHba->channel[chan].device == NULL){
> > > -		printk(KERN_DEBUG"Adaptec I2O RAID: Trying to find device before they are allocated\n");
> > > -		return NULL;
> > > -	}
> >
> > dpt_i2o is always weirdly fun, but I think, based on the message, this
> > check is supposed to be
> >
> > if( pHba->channel[chan].device[id] == NULL){
> >
> > Since device is an array of device pointers which are allocated by
> > parsing data.
> 
> That seems to be already checked immediately below:
> 
>         d = pHba->channel[chan].device[id];
> 	if(!d || d->tid == 0) {
>                 return NULL;

Yes, I know, but no message is emitted.  The message seems to be for a
violation of the state machine which device[id] = NULL implies.

James

Re: [PATCH 1/1] dpt_i2o: delete unnecessary null test on array

[Julia Lawall]

On Fri, 8 Aug 2014, James Bottomley wrote:

> On Fri, 2014-08-08 at 19:03 +0200, Julia Lawall wrote:
> > > > diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
> > > > index 67283ef..62e276b 100644
> > > > --- a/drivers/scsi/dpt_i2o.c
> > > > +++ b/drivers/scsi/dpt_i2o.c
> > > > @@ -1169,11 +1169,6 @@ static struct adpt_device* adpt_find_device(adpt_hba* pHba, u32 chan, u32 id, u6
> > > >  	if(chan < 0 || chan >= MAX_CHANNEL)
> > > >  		return NULL;
> > > >
> > > > -	if( pHba->channel[chan].device == NULL){
> > > > -		printk(KERN_DEBUG"Adaptec I2O RAID: Trying to find device before they are allocated\n");
> > > > -		return NULL;
> > > > -	}
> > >
> > > dpt_i2o is always weirdly fun, but I think, based on the message, this
> > > check is supposed to be
> > >
> > > if( pHba->channel[chan].device[id] == NULL){
> > >
> > > Since device is an array of device pointers which are allocated by
> > > parsing data.
> >
> > That seems to be already checked immediately below:
> >
> >         d = pHba->channel[chan].device[id];
> > 	if(!d || d->tid == 0) {
> >                 return NULL;
>
> Yes, I know, but no message is emitted.  The message seems to be for a
> violation of the state machine which device[id] = NULL implies.

OK, if the message seems helpful, then I can split up the tests.

julia

Re: [PATCH 1/1] dpt_i2o: delete unnecessary null test on array

[Julia Lawall]

>From nobody Sat Aug  9 08:17:15 CEST 2014
From: Julia Lawall <Julia.Lawall@lip6.fr>
To: Adaptec OEM Raid Solutions <aacraid@adaptec.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>,linux-scsi@vger.kernel.org,linux-kernel@vger.kernel.org
Subject: [PATCH] dpt_i2o: delete unnecessary null test on array

From: Julia Lawall <Julia.Lawall@lip6.fr>

Convert a NULL test on an array to a NULL test on its element.  Delete a
redundant test and clean up the code a little.

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r@
type T;
T [] e;
position p;
@@
e ==@p NULL

@ disable fld_to_ptr@
expression e;
identifier f;
position r.p;
@@
* e.f ==@p NULL
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
v2: Test instead the array element, and clean up the code a bit.

 drivers/scsi/dpt_i2o.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index 67283ef..4647c93 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -1169,15 +1169,14 @@ static struct adpt_device* adpt_find_device(adpt_hba* pHba, u32 chan, u32 id, u6
 	if(chan < 0 || chan >= MAX_CHANNEL)
 		return NULL;
 	
-	if( pHba->channel[chan].device == NULL){
-		printk(KERN_DEBUG"Adaptec I2O RAID: Trying to find device before they are allocated\n");
+	d = pHba->channel[chan].device[id];
+	if (!d) {
+		printk(KERN_DEBUG "Adaptec I2O RAID: Trying to find device before they are allocated\n");
 		return NULL;
 	}
 
-	d = pHba->channel[chan].device[id];
-	if(!d || d->tid == 0) {
+	if (d->tid == 0)
 		return NULL;
-	}
 
 	/* If it is the only lun at that address then this should match*/
 	if(d->scsi_lun == lun){