[PATCH] scsi: return correct status in scsi_host_eh_past_deadline()

[PATCH] scsi: return correct status in scsi_host_eh_past_deadline()

[Hannes Reinecke]

If the user changed the 'eh_deadline' setting to 'off' while evaluating
the time_before() call we will return 'true', which is inconsistent
with the first conditional, where we return 'false' if 'eh_deadline'
is set to 'off'.

Reported-by: Martin Wilck <martin.wilck@suse.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_error.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ae2fa170f6ad..ae29a9b4af56 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -113,7 +113,7 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 	    shost->eh_deadline > -1)
 		return 0;
 
-	return 1;
+	return shost->eh_deadline == -1 ? 0 : 1;
 }
 
 /**
-- 
2.16.4

Re: [PATCH] scsi: return correct status in scsi_host_eh_past_deadline()

[Laurence Oberman]

On Tue, 2020-02-04 at 11:23 +0100, Hannes Reinecke wrote:
> If the user changed the 'eh_deadline' setting to 'off' while
> evaluating
> the time_before() call we will return 'true', which is inconsistent
> with the first conditional, where we return 'false' if 'eh_deadline'
> is set to 'off'.
> 
> Reported-by: Martin Wilck <martin.wilck@suse.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi_error.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index ae2fa170f6ad..ae29a9b4af56 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -113,7 +113,7 @@ static int scsi_host_eh_past_deadline(struct
> Scsi_Host *shost)
>  	    shost->eh_deadline > -1)
>  		return 0;
>  
> -	return 1;
> +	return shost->eh_deadline == -1 ? 0 : 1;
>  }
>  
>  /**

Makes sense, looks right to me.
Reviewed-by: Laurence Oberman <loberman@redhat.com>

Re: [PATCH] scsi: return correct status in scsi_host_eh_past_deadline()

[Ewan D. Milne]

On Tue, 2020-02-04 at 11:23 +0100, Hannes Reinecke wrote:
> If the user changed the 'eh_deadline' setting to 'off' while evaluating
> the time_before() call we will return 'true', which is inconsistent
> with the first conditional, where we return 'false' if 'eh_deadline'
> is set to 'off'.
> 
> Reported-by: Martin Wilck <martin.wilck@suse.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi_error.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index ae2fa170f6ad..ae29a9b4af56 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -113,7 +113,7 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
>  	    shost->eh_deadline > -1)
>  		return 0;
>  
> -	return 1;
> +	return shost->eh_deadline == -1 ? 0 : 1;
>  }
>  
>  /**

Hmm.  4 accesses to shost->eh_deadline in the function?
Why don't we just copy it to a local variable and use that.

-Ewan

Re: [PATCH] scsi: return correct status in scsi_host_eh_past_deadline()

[Bart Van Assche]

On 2/4/20 2:23 AM, Hannes Reinecke wrote:
> If the user changed the 'eh_deadline' setting to 'off' while evaluating
> the time_before() call we will return 'true', which is inconsistent
> with the first conditional, where we return 'false' if 'eh_deadline'
> is set to 'off'.
> 
> Reported-by: Martin Wilck <martin.wilck@suse.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/scsi/scsi_error.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index ae2fa170f6ad..ae29a9b4af56 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -113,7 +113,7 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
>   	    shost->eh_deadline > -1)
>   		return 0;
>   
> -	return 1;
> +	return shost->eh_deadline == -1 ? 0 : 1;
>   }

This seems like a good opportunity to change the return type of this 
function from 'int' into 'bool'?

Thanks,

Bart.

Re: [PATCH] scsi: return correct status in scsi_host_eh_past_deadline()

[Bart Van Assche]

On 2/4/20 1:54 PM, Ewan D. Milne wrote:
> On Tue, 2020-02-04 at 11:23 +0100, Hannes Reinecke wrote:
>> If the user changed the 'eh_deadline' setting to 'off' while evaluating
>> the time_before() call we will return 'true', which is inconsistent
>> with the first conditional, where we return 'false' if 'eh_deadline'
>> is set to 'off'.
>>
>> Reported-by: Martin Wilck <martin.wilck@suse.com>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/scsi/scsi_error.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index ae2fa170f6ad..ae29a9b4af56 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -113,7 +113,7 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
>>   	    shost->eh_deadline > -1)
>>   		return 0;
>>   
>> -	return 1;
>> +	return shost->eh_deadline == -1 ? 0 : 1;
>>   }
>>   
>>   /**
> 
> Hmm.  4 accesses to shost->eh_deadline in the function?
> Why don't we just copy it to a local variable and use that.

That sounds like a better solution to me and that would also fix the 
problem described by Hannes.

Bart.