| Message ID | 20190731090526.27245-1-colin.king@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: remove redundant assignment to variable rc | expand |
Colin King <colin.king@canonical.com> writes: > Variable rc is being initialized with a value that is never read > and rc is being re-assigned a little later on. The assignment is > redundant and hence can be removed. I think I would actually rather have rc set to an error by default than uninitialized. Just my personal opinion. Cheers,
On Wed, Jul 31, 2019 at 12:09:31PM +0200, Aurélien Aptel wrote: > Colin King <colin.king@canonical.com> writes: > > Variable rc is being initialized with a value that is never read > > and rc is being re-assigned a little later on. The assignment is > > redundant and hence can be removed. > > I think I would actually rather have rc set to an error by default than > uninitialized. Just my personal opinion. You're just turning off GCC's static analysis (and introducing false positives) when you do that. We have seen bugs caused by this and never seen any bugs prevented by this style. regards, dan carpenter
"Dan Carpenter" <dan.carpenter@oracle.com> writes: > You're just turning off GCC's static analysis (and introducing false > positives) when you do that. We have seen bugs caused by this and never > seen any bugs prevented by this style. You've never seen bugs prevented by initializing uninitialized variables? Code can change overtime and I don't think coverity is checked as often as it could be, meaning the var could end up being used while uninitialized in the future. Anyway I won't die on this hill, merge this if you prefer. Cheers,
On 31/07/2019 16:34, Aurélien Aptel wrote: > "Dan Carpenter" <dan.carpenter@oracle.com> writes: >> You're just turning off GCC's static analysis (and introducing false >> positives) when you do that. We have seen bugs caused by this and never >> seen any bugs prevented by this style. > > You've never seen bugs prevented by initializing uninitialized > variables? Code can change overtime and I don't think coverity is > checked as often as it could be, meaning the var could end up being used > while uninitialized in the future. gcc/clang should pick up uninitialized vars at compile time. also I run coverity daily on linux-next. Colin > > Anyway I won't die on this hill, merge this if you prefer. > > Cheers, >
On Wed, Jul 31, 2019 at 05:34:39PM +0200, Aurélien Aptel wrote: > "Dan Carpenter" <dan.carpenter@oracle.com> writes: > > You're just turning off GCC's static analysis (and introducing false > > positives) when you do that. We have seen bugs caused by this and never > > seen any bugs prevented by this style. > > You've never seen bugs prevented by initializing uninitialized > variables? Code can change overtime and I don't think coverity is > checked as often as it could be, meaning the var could end up being used > while uninitialized in the future. Of course, we wouldn't see bugs that were prevented so that wasn't entirely fair. There is a several year old bug in GCC where it sometimes initializes these to zero and doesn't warn about the uninitialized variable so it is actually possible to prevent a bug by initializing it to an error code. Smatch also warns about uninitialized variables. I normally run Smatch on linux-next every day but I have been out of office for the past month and my config doesn't cover everything. We haven't been able to enable this "redundant assignment" warning because of all the false positives like this. It mostly finds dead code but it also does find some bugs where we forget to check the error code or we use the wrong variable. regards, dan carpenter
merged into cifs-2.6.git for-next On Wed, Jul 31, 2019 at 10:54 AM Colin Ian King <colin.king@canonical.com> wrote: > > On 31/07/2019 16:34, Aurélien Aptel wrote: > > "Dan Carpenter" <dan.carpenter@oracle.com> writes: > >> You're just turning off GCC's static analysis (and introducing false > >> positives) when you do that. We have seen bugs caused by this and never > >> seen any bugs prevented by this style. > > > > You've never seen bugs prevented by initializing uninitialized > > variables? Code can change overtime and I don't think coverity is > > checked as often as it could be, meaning the var could end up being used > > while uninitialized in the future. > > gcc/clang should pick up uninitialized vars at compile time. also I run > coverity daily on linux-next. > > Colin > > > > > Anyway I won't die on this hill, merge this if you prefer. > > > > Cheers, > > >
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index ee5d74988a9f..a653c429e8dc 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -3617,7 +3617,7 @@ SMB2_read(const unsigned int xid, struct cifs_io_parms *io_parms, unsigned int *nbytes, char **buf, int *buf_type) { struct smb_rqst rqst; - int resp_buftype, rc = -EACCES; + int resp_buftype, rc; struct smb2_read_plain_req *req = NULL; struct smb2_read_rsp *rsp = NULL; struct kvec iov[1];