| Message ID | 20200204102316.39000-1-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: return correct status in scsi_host_eh_past_deadline() | expand |
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>
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
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.
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.
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; } /**
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(-)