From: Colin Ian King <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]> wrote:
>
> From: Colin Ian King <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>
On Sat, May 11, 2019 at 02:27:00PM +0100, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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
>> 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 <[email protected]> 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
>
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
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