From: Alan Stern <stern@rowland.harvard.edu>
To: Tom Rix <trix@redhat.com>
Cc: gregkh@linuxfoundation.org, acozzette@cs.hmc.edu,
linux-usb@vger.kernel.org, usb-storage@lists.one-eyed-alien.net,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] USB: realtek_cr: fix return check for dma functions
Date: Tue, 11 Aug 2020 15:43:19 -0400 [thread overview]
Message-ID: <20200811194319.GB344152@rowland.harvard.edu> (raw)
In-Reply-To: <c48fec19-fe2c-65c6-917b-8b8ba40e4c7e@redhat.com>
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
prev parent reply other threads:[~2020-08-11 19:43 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-11 15:15 [PATCH] USB: realtek_cr: fix return check for dma functions trix
2020-08-11 16:03 ` Alan Stern
2020-08-11 17:29 ` Tom Rix
2020-08-11 17:53 ` Alan Stern
2020-08-11 18:54 ` Tom Rix
2020-08-11 19:43 ` Alan Stern [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200811194319.GB344152@rowland.harvard.edu \
--to=stern@rowland.harvard.edu \
--cc=acozzette@cs.hmc.edu \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=trix@redhat.com \
--cc=usb-storage@lists.one-eyed-alien.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).