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
