[PATCH] orangefs: remove redundant assignment to variable buffer_index

[PATCH] orangefs: remove redundant assignment to variable buffer_index

[Colin Ian King]

The variable buffer_index is assigned a value that is never read,
it is assigned just before the function returns. The assignment is
redundant and can be removed.

Cleans up clang scan build warning:
fs/orangefs/file.c:276:3: warning: Value stored to 'buffer_index'
is never read [deadcode.DeadStores]

Signed-off-by: Colin Ian King <colin.i.king@gmail.com>
---
 fs/orangefs/file.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index 732661aa2680..167fa43b24f9 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -273,7 +273,6 @@ ssize_t wait_for_direct_io(enum ORANGEFS_io_type type, struct inode *inode,
 		gossip_debug(GOSSIP_FILE_DEBUG,
 			"%s(%pU): PUT buffer_index %d\n",
 			__func__, handle, buffer_index);
-		buffer_index = -1;
 	}
 	op_release(new_op);
 	return ret;
-- 
2.37.3

[PATCH] orangefs: remove redundant assignment to variable buffer_index

[Colin King]

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

The variable buffer_index is being initialized however this is never
read and later it is being reassigned to a new value. The initialization
is redundant and hence can be removed.

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

diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index a35c17017210..80f06ee794c5 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -52,7 +52,7 @@ ssize_t wait_for_direct_io(enum ORANGEFS_io_type type, struct inode *inode,
 	struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
 	struct orangefs_khandle *handle = &orangefs_inode->refn.khandle;
 	struct orangefs_kernel_op_s *new_op = NULL;
-	int buffer_index = -1;
+	int buffer_index;
 	ssize_t ret;
 	size_t copy_amount;
 
-- 
2.20.1

[PATCH] orangefs: remove redundant assignment to variable buffer_index

[Colin King]

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

The variable buffer_index is being initialized however this is never
read and later it is being reassigned to a new value. The initialization
is redundant and hence can be removed.

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

diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index a35c17017210..80f06ee794c5 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -52,7 +52,7 @@ ssize_t wait_for_direct_io(enum ORANGEFS_io_type type, struct inode *inode,
 	struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
 	struct orangefs_khandle *handle = &orangefs_inode->refn.khandle;
 	struct orangefs_kernel_op_s *new_op = NULL;
-	int buffer_index = -1;
+	int buffer_index;
 	ssize_t ret;
 	size_t copy_amount;
 
-- 
2.20.1

Re: [PATCH] orangefs: remove redundant assignment to variable buffer_index

[Mike Marshall]

Hi Colin...

Thanks for the patch. Before I initialized buffer_index, Dan Williams sent
in a warning that a particular error path could try to use ibuffer_index
uninitialized. I could induce the problem he described with one
of the xfstests resulting in a crashed kernel. I will try to refactor
the code to fix the problem some other way than initializing
buffer_index in the declaration.

-Mike

On Sat, May 11, 2019 at 9:27 AM Colin King <colin.king@canonical.com> wrote:
>
> From: Colin Ian King <colin.king@canonical.com>
>
> The variable buffer_index is being initialized however this is never
> read and later it is being reassigned to a new value. The initialization
> is redundant and hence can be removed.
>
> Addresses-Coverity: ("Unused Value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  fs/orangefs/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
> index a35c17017210..80f06ee794c5 100644
> --- a/fs/orangefs/file.c
> +++ b/fs/orangefs/file.c
> @@ -52,7 +52,7 @@ ssize_t wait_for_direct_io(enum ORANGEFS_io_type type, struct inode *inode,
>         struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
>         struct orangefs_khandle *handle = &orangefs_inode->refn.khandle;
>         struct orangefs_kernel_op_s *new_op = NULL;
> -       int buffer_index = -1;
> +       int buffer_index;
>         ssize_t ret;
>         size_t copy_amount;
>
> --
> 2.20.1
>

Re: [PATCH] orangefs: remove redundant assignment to variable buffer_index

[Mike Marshall]

Hi Colin...

Thanks for the patch. Before I initialized buffer_index, Dan Williams sent
in a warning that a particular error path could try to use ibuffer_index
uninitialized. I could induce the problem he described with one
of the xfstests resulting in a crashed kernel. I will try to refactor
the code to fix the problem some other way than initializing
buffer_index in the declaration.

-Mike

On Sat, May 11, 2019 at 9:27 AM Colin King <colin.king@canonical.com> wrote:
>
> From: Colin Ian King <colin.king@canonical.com>
>
> The variable buffer_index is being initialized however this is never
> read and later it is being reassigned to a new value. The initialization
> is redundant and hence can be removed.
>
> Addresses-Coverity: ("Unused Value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  fs/orangefs/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
> index a35c17017210..80f06ee794c5 100644
> --- a/fs/orangefs/file.c
> +++ b/fs/orangefs/file.c
> @@ -52,7 +52,7 @@ ssize_t wait_for_direct_io(enum ORANGEFS_io_type type, struct inode *inode,
>         struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
>         struct orangefs_khandle *handle = &orangefs_inode->refn.khandle;
>         struct orangefs_kernel_op_s *new_op = NULL;
> -       int buffer_index = -1;
> +       int buffer_index;
>         ssize_t ret;
>         size_t copy_amount;
>
> --
> 2.20.1
>

Re: [PATCH] orangefs: remove redundant assignment to variable buffer_index

[Dan Carpenter]

On Thu, May 16, 2019 at 12:06:31PM -0400, Mike Marshall wrote:
> Hi Colin...
> 
> Thanks for the patch. Before I initialized buffer_index, Dan Williams sent
> in a warning that a particular error path could try to use ibuffer_index
> uninitialized. I could induce the problem he described with one
> of the xfstests resulting in a crashed kernel. I will try to refactor
> the code to fix the problem some other way than initializing
> buffer_index in the declaration.
> 

The only explanation I can think of is that you guys are discussing
different code.  :P

regards,
dan carpenter

Re: [PATCH] orangefs: remove redundant assignment to variable buffer_index

[Dan Carpenter]

On Thu, May 16, 2019 at 12:06:31PM -0400, Mike Marshall wrote:
> Hi Colin...
> 
> Thanks for the patch. Before I initialized buffer_index, Dan Williams sent
> in a warning that a particular error path could try to use ibuffer_index
> uninitialized. I could induce the problem he described with one
> of the xfstests resulting in a crashed kernel. I will try to refactor
> the code to fix the problem some other way than initializing
> buffer_index in the declaration.
> 

The only explanation I can think of is that you guys are discussing
different code.  :P

regards,
dan carpenter

Re: [PATCH] orangefs: remove redundant assignment to variable buffer_index

[Mike Marshall]

>> The only explanation I can think of is that you guys are discussing
>> different code. :P

My response contained several conflations :-) ...

The code in file.c that Colin has flagged does indeed have buffer_index
being initialized needlessly, and the assignment noted by Dan is also
needless. There's even a second needless assignment done in another
place in the same function. While the code around them has changed over
time, these now needless manipulations of buffer_index are not new. I'll
get rid of them.

>> You often send these patches before they hit linux-next so I had skipped
>> reviewing this one when you sent it.

I know Linus is likely to refuse pull requests for stuff that
has not been through linux-next, so I make sure stuff has been
there at least a few days before asking for it to be pulled.
"A few days" is long enough for robots to see it, perhaps not
long enough for humans. I especially appreciate the human review. One of
the good things about Orangefs is that it is easy to install and configure,
especially for testing. Documentation/filesystems/orangefs.txt has
instructions for dnf installing orangefs on Fedora, and also how to download
a source tarball and install from that.

-Mike

On Tue, May 21, 2019 at 11:04 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Thu, May 16, 2019 at 12:06:31PM -0400, Mike Marshall wrote:
> > Hi Colin...
> >
> > Thanks for the patch. Before I initialized buffer_index, Dan Williams sent
> > in a warning that a particular error path could try to use ibuffer_index
> > uninitialized. I could induce the problem he described with one
> > of the xfstests resulting in a crashed kernel. I will try to refactor
> > the code to fix the problem some other way than initializing
> > buffer_index in the declaration.
> >
>
> The only explanation I can think of is that you guys are discussing
> different code.  :P
>
> regards,
> dan carpenter
>

Re: [PATCH] orangefs: remove redundant assignment to variable buffer_index

[Mike Marshall]

>> The only explanation I can think of is that you guys are discussing
>> different code. :P

My response contained several conflations :-) ...

The code in file.c that Colin has flagged does indeed have buffer_index
being initialized needlessly, and the assignment noted by Dan is also
needless. There's even a second needless assignment done in another
place in the same function. While the code around them has changed over
time, these now needless manipulations of buffer_index are not new. I'll
get rid of them.

>> You often send these patches before they hit linux-next so I had skipped
>> reviewing this one when you sent it.

I know Linus is likely to refuse pull requests for stuff that
has not been through linux-next, so I make sure stuff has been
there at least a few days before asking for it to be pulled.
"A few days" is long enough for robots to see it, perhaps not
long enough for humans. I especially appreciate the human review. One of
the good things about Orangefs is that it is easy to install and configure,
especially for testing. Documentation/filesystems/orangefs.txt has
instructions for dnf installing orangefs on Fedora, and also how to download
a source tarball and install from that.

-Mike

On Tue, May 21, 2019 at 11:04 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Thu, May 16, 2019 at 12:06:31PM -0400, Mike Marshall wrote:
> > Hi Colin...
> >
> > Thanks for the patch. Before I initialized buffer_index, Dan Williams sent
> > in a warning that a particular error path could try to use ibuffer_index
> > uninitialized. I could induce the problem he described with one
> > of the xfstests resulting in a crashed kernel. I will try to refactor
> > the code to fix the problem some other way than initializing
> > buffer_index in the declaration.
> >
>
> The only explanation I can think of is that you guys are discussing
> different code.  :P
>
> regards,
> dan carpenter
>

Re: [PATCH] orangefs: remove redundant assignment to variable buffer_index

[Dan Carpenter]

On Tue, Jun 25, 2019 at 02:55:11PM -0400, Mike Marshall wrote:
> >> You often send these patches before they hit linux-next so I had skipped
> >> reviewing this one when you sent it.
> 
> I know Linus is likely to refuse pull requests for stuff that
> has not been through linux-next, so I make sure stuff has been
> there at least a few days before asking for it to be pulled.
> "A few days" is long enough for robots to see it, perhaps not
> long enough for humans. I especially appreciate the human review. One of
> the good things about Orangefs is that it is easy to install and configure,
> especially for testing. Documentation/filesystems/orangefs.txt has
> instructions for dnf installing orangefs on Fedora, and also how to download
> a source tarball and install from that.

No, no, that comment was to Colin.  It's good that he's sending patches
for all the trees as soon as possible like the zero day bot does.  But
it does make it hard to review at times.

regards,
dan carpenter

Re: [PATCH] orangefs: remove redundant assignment to variable buffer_index

[Dan Carpenter]

On Tue, Jun 25, 2019 at 02:55:11PM -0400, Mike Marshall wrote:
> >> You often send these patches before they hit linux-next so I had skipped
> >> reviewing this one when you sent it.
> 
> I know Linus is likely to refuse pull requests for stuff that
> has not been through linux-next, so I make sure stuff has been
> there at least a few days before asking for it to be pulled.
> "A few days" is long enough for robots to see it, perhaps not
> long enough for humans. I especially appreciate the human review. One of
> the good things about Orangefs is that it is easy to install and configure,
> especially for testing. Documentation/filesystems/orangefs.txt has
> instructions for dnf installing orangefs on Fedora, and also how to download
> a source tarball and install from that.

No, no, that comment was to Colin.  It's good that he's sending patches
for all the trees as soon as possible like the zero day bot does.  But
it does make it hard to review at times.

regards,
dan carpenter

Re: [PATCH] orangefs: remove redundant assignment to variable buffer_index

[Colin Ian King]

On 26/06/2019 07:18, Dan Carpenter wrote:
> On Tue, Jun 25, 2019 at 02:55:11PM -0400, Mike Marshall wrote:
>>>> You often send these patches before they hit linux-next so I had skipped
>>>> reviewing this one when you sent it.
>>
>> I know Linus is likely to refuse pull requests for stuff that
>> has not been through linux-next, so I make sure stuff has been
>> there at least a few days before asking for it to be pulled.
>> "A few days" is long enough for robots to see it, perhaps not
>> long enough for humans. I especially appreciate the human review. One of
>> the good things about Orangefs is that it is easy to install and configure,
>> especially for testing. Documentation/filesystems/orangefs.txt has
>> instructions for dnf installing orangefs on Fedora, and also how to download
>> a source tarball and install from that.
> 
> No, no, that comment was to Colin.  It's good that he's sending patches
> for all the trees as soon as possible like the zero day bot does.  But
> it does make it hard to review at times.
> 
> regards,
> dan carpenter
> 
Indeed, I normally work against the latest code landing in linux-next,
so apologies for any confusion.  Anyhow, by the look of it these minor
nitpicks still need addressing (really low priority), so shall I leave
that with you Mike to sort out?

Colin

Re: [PATCH] orangefs: remove redundant assignment to variable buffer_index

[Colin Ian King]

On 26/06/2019 07:18, Dan Carpenter wrote:
> On Tue, Jun 25, 2019 at 02:55:11PM -0400, Mike Marshall wrote:
>>>> You often send these patches before they hit linux-next so I had skipped
>>>> reviewing this one when you sent it.
>>
>> I know Linus is likely to refuse pull requests for stuff that
>> has not been through linux-next, so I make sure stuff has been
>> there at least a few days before asking for it to be pulled.
>> "A few days" is long enough for robots to see it, perhaps not
>> long enough for humans. I especially appreciate the human review. One of
>> the good things about Orangefs is that it is easy to install and configure,
>> especially for testing. Documentation/filesystems/orangefs.txt has
>> instructions for dnf installing orangefs on Fedora, and also how to download
>> a source tarball and install from that.
> 
> No, no, that comment was to Colin.  It's good that he's sending patches
> for all the trees as soon as possible like the zero day bot does.  But
> it does make it hard to review at times.
> 
> regards,
> dan carpenter
> 
Indeed, I normally work against the latest code landing in linux-next,
so apologies for any confusion.  Anyhow, by the look of it these minor
nitpicks still need addressing (really low priority), so shall I leave
that with you Mike to sort out?

Colin

Re: [PATCH] orangefs: remove redundant assignment to variable buffer_index

[Dan Carpenter]

On Sat, May 11, 2019 at 02:27:00PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The variable buffer_index is being initialized however this is never
> read and later it is being reassigned to a new value. The initialization
> is redundant and hence can be removed.
> 
> Addresses-Coverity: ("Unused Value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  fs/orangefs/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
> index a35c17017210..80f06ee794c5 100644
> --- a/fs/orangefs/file.c
> +++ b/fs/orangefs/file.c
> @@ -52,7 +52,7 @@ ssize_t wait_for_direct_io(enum ORANGEFS_io_type type, struct inode *inode,
>  	struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
>  	struct orangefs_khandle *handle = &orangefs_inode->refn.khandle;
>  	struct orangefs_kernel_op_s *new_op = NULL;
> -	int buffer_index = -1;
> +	int buffer_index;
>  	ssize_t ret;
>  	size_t copy_amount;
>  

There is a second pointless assignment at the end of the function as
well:

   247  
   248          ret = new_op->downcall.resp.io.amt_complete;
   249  
   250  out:
   251          if (buffer_index >= 0) {
   252                  if ((readahead_size) && (type == ORANGEFS_IO_READ)) {
   253                          /* readpage */
   254                          *index_return = buffer_index;
   255                          gossip_debug(GOSSIP_FILE_DEBUG,
   256                                  "%s: hold on to buffer_index :%d:\n",
   257                                  __func__, buffer_index);
   258                  } else {
   259                          /* O_DIRECT */
   260                          orangefs_bufmap_put(buffer_index);
   261                          gossip_debug(GOSSIP_FILE_DEBUG,
   262                                  "%s(%pU): PUT buffer_index %d\n",
   263                                  __func__, handle, buffer_index);
   264                  }
   265                  buffer_index = -1;
                        ^^^^^^^^^^^^^^^^^

   266          }
   267          op_release(new_op);
   268          return ret;
   269  }

You often send these patches before they hit linux-next so I had skipped
reviewing this one when you sent it.  I'm coming back to work today
after the flu so I was going through my inbox reviewing old unread
messages...

regards,
dan carpenter

Re: [PATCH] orangefs: remove redundant assignment to variable buffer_index

[Dan Carpenter]

On Sat, May 11, 2019 at 02:27:00PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The variable buffer_index is being initialized however this is never
> read and later it is being reassigned to a new value. The initialization
> is redundant and hence can be removed.
> 
> Addresses-Coverity: ("Unused Value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  fs/orangefs/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
> index a35c17017210..80f06ee794c5 100644
> --- a/fs/orangefs/file.c
> +++ b/fs/orangefs/file.c
> @@ -52,7 +52,7 @@ ssize_t wait_for_direct_io(enum ORANGEFS_io_type type, struct inode *inode,
>  	struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
>  	struct orangefs_khandle *handle = &orangefs_inode->refn.khandle;
>  	struct orangefs_kernel_op_s *new_op = NULL;
> -	int buffer_index = -1;
> +	int buffer_index;
>  	ssize_t ret;
>  	size_t copy_amount;
>  

There is a second pointless assignment at the end of the function as
well:

   247  
   248          ret = new_op->downcall.resp.io.amt_complete;
   249  
   250  out:
   251          if (buffer_index >= 0) {
   252                  if ((readahead_size) && (type = ORANGEFS_IO_READ)) {
   253                          /* readpage */
   254                          *index_return = buffer_index;
   255                          gossip_debug(GOSSIP_FILE_DEBUG,
   256                                  "%s: hold on to buffer_index :%d:\n",
   257                                  __func__, buffer_index);
   258                  } else {
   259                          /* O_DIRECT */
   260                          orangefs_bufmap_put(buffer_index);
   261                          gossip_debug(GOSSIP_FILE_DEBUG,
   262                                  "%s(%pU): PUT buffer_index %d\n",
   263                                  __func__, handle, buffer_index);
   264                  }
   265                  buffer_index = -1;
                        ^^^^^^^^^^^^^^^^^

   266          }
   267          op_release(new_op);
   268          return ret;
   269  }

You often send these patches before they hit linux-next so I had skipped
reviewing this one when you sent it.  I'm coming back to work today
after the flu so I was going through my inbox reviewing old unread
messages...

regards,
dan carpenter