Messages in this thread | | | Date | Tue, 11 Aug 2020 15:43:19 -0400 | From | Alan Stern <> | Subject | Re: [PATCH] USB: realtek_cr: fix return check for dma functions |
| |
On Tue, Aug 11, 2020 at 11:54:28AM -0700, Tom Rix wrote: > > On 8/11/20 10:53 AM, Alan Stern wrote:
> >>> Instead of changing all these call sites, wouldn't it be a lot easier > >>> just to change rts51x_read_mem() to make it always return a negative > >>> value (such as -EIO) when there's an error? > >>> > >>> Alan Stern > >> I thought about that but there was already existing (retval != > >> STATUS_SUCCESS) checks for these calls. > > The only values that routine currently returns are > > USB_STOR_TRANSPORT_ERROR, -EIO, and 0. None of the callers distinguish > > between the first two values, so you can just change the first to the > > second. > > > > Note that STATUS_SUCCESS is simply 0. > > Yes, i noted all of these already. My change is consistent with the > existing correct checks. consistency is important. returning a neg > value to reuse the exiting check should mean the STATUS_SUCCESS != 0 > checks are changed to neg check.
Do you mean the "retval == STATUS_SUCCESS" checks? Those checks would end up doing exactly the same thing as they do now, since USB_STOR_TRANSPORT_ERROR and -EIO are both different from 0.
Yes, it is true that consistency is important. But correctness is more important than consistency.
> i can do this larger change if > required.
Let me put it this way: Suppose you changed the USB_STOR_TRANSPORT_ERROR in rts51x_read_mem() to -EIO, without changing anything else. Wouldn't that fix the problem reported by the clang static analysis? If not, why not?
Alan Stern
| |