Linux-CIFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] cifs: remove redundant assignment to variable rc
@ 2019-07-31  9:05 Colin King
  2019-07-31 10:09 ` Aurélien Aptel
  0 siblings, 1 reply; 7+ messages in thread
From: Colin King @ 2019-07-31  9:05 UTC (permalink / raw)
  To: Steve French, linux-cifs, samba-technical; +Cc: kernel-janitors, linux-kernel

From: Colin Ian King <colin.king@canonical.com>

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.

Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 fs/cifs/smb2pdu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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];
-- 
2.20.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] cifs: remove redundant assignment to variable rc
  2019-07-31  9:05 [PATCH] cifs: remove redundant assignment to variable rc Colin King
@ 2019-07-31 10:09 ` Aurélien Aptel
  2019-07-31 12:28   ` Dan Carpenter
  0 siblings, 1 reply; 7+ messages in thread
From: Aurélien Aptel @ 2019-07-31 10:09 UTC (permalink / raw)
  To: Colin King, samba-technical, Steve French, linux-cifs
  Cc: kernel-janitors, linux-kernel

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,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] cifs: remove redundant assignment to variable rc
  2019-07-31 10:09 ` Aurélien Aptel
@ 2019-07-31 12:28   ` Dan Carpenter
  2019-07-31 15:34     ` Aurélien Aptel
  0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2019-07-31 12:28 UTC (permalink / raw)
  To: Aurélien Aptel
  Cc: Colin King, samba-technical, Steve French, linux-cifs,
	kernel-janitors, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] cifs: remove redundant assignment to variable rc
  2019-07-31 12:28   ` Dan Carpenter
@ 2019-07-31 15:34     ` Aurélien Aptel
  2019-07-31 15:54       ` Colin Ian King
  2019-08-01  8:03       ` Dan Carpenter
  0 siblings, 2 replies; 7+ messages in thread
From: Aurélien Aptel @ 2019-07-31 15:34 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Colin King, samba-technical, Steve French, kernel-janitors,
	linux-cifs, linux-kernel

"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,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] cifs: remove redundant assignment to variable rc
  2019-07-31 15:34     ` Aurélien Aptel
@ 2019-07-31 15:54       ` Colin Ian King
  2019-08-01 17:00         ` Steve French
  2019-08-01  8:03       ` Dan Carpenter
  1 sibling, 1 reply; 7+ messages in thread
From: Colin Ian King @ 2019-07-31 15:54 UTC (permalink / raw)
  To: Aurélien Aptel, Dan Carpenter
  Cc: samba-technical, Steve French, kernel-janitors, linux-cifs, linux-kernel

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,
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] cifs: remove redundant assignment to variable rc
  2019-07-31 15:34     ` Aurélien Aptel
  2019-07-31 15:54       ` Colin Ian King
@ 2019-08-01  8:03       ` Dan Carpenter
  1 sibling, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2019-08-01  8:03 UTC (permalink / raw)
  To: Aurélien Aptel
  Cc: Colin King, samba-technical, Steve French, kernel-janitors,
	linux-cifs, linux-kernel

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] cifs: remove redundant assignment to variable rc
  2019-07-31 15:54       ` Colin Ian King
@ 2019-08-01 17:00         ` Steve French
  0 siblings, 0 replies; 7+ messages in thread
From: Steve French @ 2019-08-01 17:00 UTC (permalink / raw)
  To: Colin Ian King
  Cc: Aurélien Aptel, Dan Carpenter, samba-technical,
	Steve French, kernel-janitors, CIFS, LKML

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,
> >
>


-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-31  9:05 [PATCH] cifs: remove redundant assignment to variable rc Colin King
2019-07-31 10:09 ` Aurélien Aptel
2019-07-31 12:28   ` Dan Carpenter
2019-07-31 15:34     ` Aurélien Aptel
2019-07-31 15:54       ` Colin Ian King
2019-08-01 17:00         ` Steve French
2019-08-01  8:03       ` Dan Carpenter

Linux-CIFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cifs/0 linux-cifs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cifs linux-cifs/ https://lore.kernel.org/linux-cifs \
		linux-cifs@vger.kernel.org linux-cifs@archiver.kernel.org
	public-inbox-index linux-cifs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cifs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox