* [PATCH] fs/reiserfs/inode: remove dead code in _get_block_create_0()
@ 2022-07-20 6:33 zengjx95
2022-07-20 6:57 ` Al Viro
0 siblings, 1 reply; 4+ messages in thread
From: zengjx95 @ 2022-07-20 6:33 UTC (permalink / raw)
To: reiserfs-devel
Cc: willy, jlayton, damien.lemoal, jack, edward.shishkin,
linux-kernel, Zeng Jingxiang
From: Zeng Jingxiang <linuszeng@tencent.com>
Fix "control flow" issues found by Coverity
Logically dead code (DEADCODE)
Execution cannot reach this statement.
Assigned_value: Assigning value NULL to p here
293 char *p = NULL;
In the following conditional expression, the value of p is always NULL,
as a result, the kunmap() cannot be executed.
308 if (p)
309 kunmap(bh_result->b_page);
355 if (p)
356 kunmap(bh_result->b_page);
366 if (p)
367 kunmap(bh_result->b_page);
Signed-off-by: Zeng Jingxiang <linuszeng@tencent.com>
---
fs/reiserfs/inode.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index 0cffe054b78e..d1b0c7645fcb 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -305,8 +305,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
result = search_for_position_by_key(inode->i_sb, &key, &path);
if (result != POSITION_FOUND) {
pathrelse(&path);
- if (p)
- kunmap(bh_result->b_page);
if (result == IO_ERROR)
return -EIO;
/*
@@ -352,8 +350,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
}
pathrelse(&path);
- if (p)
- kunmap(bh_result->b_page);
return ret;
}
/* requested data are in direct item(s) */
@@ -363,8 +359,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
* when it is stored in direct item(s)
*/
pathrelse(&path);
- if (p)
- kunmap(bh_result->b_page);
return -ENOENT;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] fs/reiserfs/inode: remove dead code in _get_block_create_0()
2022-07-20 6:33 [PATCH] fs/reiserfs/inode: remove dead code in _get_block_create_0() zengjx95
@ 2022-07-20 6:57 ` Al Viro
0 siblings, 0 replies; 4+ messages in thread
From: Al Viro @ 2022-07-20 6:57 UTC (permalink / raw)
To: zengjx95
Cc: reiserfs-devel, willy, jlayton, damien.lemoal, jack,
edward.shishkin, linux-kernel, Zeng Jingxiang
On Wed, Jul 20, 2022 at 02:33:10PM +0800, zengjx95@gmail.com wrote:
> From: Zeng Jingxiang <linuszeng@tencent.com>
>
> Fix "control flow" issues found by Coverity
> Logically dead code (DEADCODE)
> Execution cannot reach this statement.
>
> Assigned_value: Assigning value NULL to p here
> 293 char *p = NULL;
> In the following conditional expression, the value of p is always NULL,
> as a result, the kunmap() cannot be executed.
> 308 if (p)
> 309 kunmap(bh_result->b_page);
>
> 355 if (p)
> 356 kunmap(bh_result->b_page);
>
> 366 if (p)
> 367 kunmap(bh_result->b_page);
>
> Signed-off-by: Zeng Jingxiang <linuszeng@tencent.com>
First of all, if you find something like that, it might be a good idea to
find _when_ had that appeared. If nothing else, transformation might
very well turn out to be obfuscating a preexisting bug.
In this case, it's not hard to find: 27b3a5c51b50 "kill-the-bkl/reiserfs:
drop the fs race watchdog from _get_block_create_0()", which had
removed a label upstream of these tests and a branch to it from
downstream of assignment to p.
Assignment survives, BTW, in the following form:
if (!p)
p = (char *)kmap(bh_result->b_page);
and this test is just as constant as the ones you'd removed. Unlike
them it's constantly true, of course, but just as inexplicable by
the current form of function.
If anything, I would suggest losing initialization of p to NULL
and making the assignment quoted above unconditional.
> ---
> fs/reiserfs/inode.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
> index 0cffe054b78e..d1b0c7645fcb 100644
> --- a/fs/reiserfs/inode.c
> +++ b/fs/reiserfs/inode.c
> @@ -305,8 +305,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
> result = search_for_position_by_key(inode->i_sb, &key, &path);
> if (result != POSITION_FOUND) {
> pathrelse(&path);
> - if (p)
> - kunmap(bh_result->b_page);
> if (result == IO_ERROR)
> return -EIO;
> /*
> @@ -352,8 +350,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
> }
>
> pathrelse(&path);
> - if (p)
> - kunmap(bh_result->b_page);
> return ret;
> }
> /* requested data are in direct item(s) */
> @@ -363,8 +359,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
> * when it is stored in direct item(s)
> */
> pathrelse(&path);
> - if (p)
> - kunmap(bh_result->b_page);
> return -ENOENT;
> }
>
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fs/reiserfs/inode: remove dead code in _get_block_create_0()
2022-07-20 8:30 zengjx95
@ 2022-07-26 10:48 ` Jan Kara
0 siblings, 0 replies; 4+ messages in thread
From: Jan Kara @ 2022-07-26 10:48 UTC (permalink / raw)
To: zengjx95
Cc: reiserfs-devel, willy, jlayton, damien.lemoal, jack,
edward.shishkin, linux-kernel, viro, kasong, Zeng Jingxiang
On Wed 20-07-22 16:30:29, zengjx95@gmail.com wrote:
> From: Zeng Jingxiang <linuszeng@tencent.com>
>
> Since commit 27b3a5c51b50 ("kill-the-bkl/reiserfs: drop the fs race
> watchdog from _get_block_create_0()"), which removed a label that may
> have the pointer 'p' touched in its control flow, related if statements
> now eval to constant value now. Just remove them.
>
> Assigning value NULL to p here
> 293 char *p = NULL;
>
> In the following conditional expression, the value of p is always NULL,
> As a result, the kunmap() cannot be executed.
> 308 if (p)
> 309 kunmap(bh_result->b_page);
>
> 355 if (p)
> 356 kunmap(bh_result->b_page);
>
> 366 if (p)
> 367 kunmap(bh_result->b_page);
>
> Also, the kmap() cannot be executed.
> 399 if (!p)
> 400 p = (char *)kmap(bh_result->b_page);
>
> Signed-off-by: Zeng Jingxiang <linuszeng@tencent.com>
> Signed-off-by: Kairui Song <kasong@tencent.com>
Thanks! I've added the patch to my tree. I've also removed the unnecessary
initialization of 'p' to NULL. I'll push the patch to Linus in the coming
merge window.
Honza
> ---
> fs/reiserfs/inode.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
> index 0cffe054b78e..fe26e1746af9 100644
> --- a/fs/reiserfs/inode.c
> +++ b/fs/reiserfs/inode.c
> @@ -305,8 +305,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
> result = search_for_position_by_key(inode->i_sb, &key, &path);
> if (result != POSITION_FOUND) {
> pathrelse(&path);
> - if (p)
> - kunmap(bh_result->b_page);
> if (result == IO_ERROR)
> return -EIO;
> /*
> @@ -352,8 +350,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
> }
>
> pathrelse(&path);
> - if (p)
> - kunmap(bh_result->b_page);
> return ret;
> }
> /* requested data are in direct item(s) */
> @@ -363,8 +359,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
> * when it is stored in direct item(s)
> */
> pathrelse(&path);
> - if (p)
> - kunmap(bh_result->b_page);
> return -ENOENT;
> }
>
> @@ -396,9 +390,7 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
> * sure we need to. But, this means the item might move if
> * kmap schedules
> */
> - if (!p)
> - p = (char *)kmap(bh_result->b_page);
> -
> + p = (char *)kmap(bh_result->b_page);
> p += offset;
> memset(p, 0, inode->i_sb->s_blocksize);
> do {
> --
> 2.27.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] fs/reiserfs/inode: remove dead code in _get_block_create_0()
@ 2022-07-20 8:30 zengjx95
2022-07-26 10:48 ` Jan Kara
0 siblings, 1 reply; 4+ messages in thread
From: zengjx95 @ 2022-07-20 8:30 UTC (permalink / raw)
To: reiserfs-devel
Cc: willy, jlayton, damien.lemoal, jack, edward.shishkin,
linux-kernel, viro, kasong, Zeng Jingxiang
From: Zeng Jingxiang <linuszeng@tencent.com>
Since commit 27b3a5c51b50 ("kill-the-bkl/reiserfs: drop the fs race
watchdog from _get_block_create_0()"), which removed a label that may
have the pointer 'p' touched in its control flow, related if statements
now eval to constant value now. Just remove them.
Assigning value NULL to p here
293 char *p = NULL;
In the following conditional expression, the value of p is always NULL,
As a result, the kunmap() cannot be executed.
308 if (p)
309 kunmap(bh_result->b_page);
355 if (p)
356 kunmap(bh_result->b_page);
366 if (p)
367 kunmap(bh_result->b_page);
Also, the kmap() cannot be executed.
399 if (!p)
400 p = (char *)kmap(bh_result->b_page);
Signed-off-by: Zeng Jingxiang <linuszeng@tencent.com>
Signed-off-by: Kairui Song <kasong@tencent.com>
---
fs/reiserfs/inode.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index 0cffe054b78e..fe26e1746af9 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -305,8 +305,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
result = search_for_position_by_key(inode->i_sb, &key, &path);
if (result != POSITION_FOUND) {
pathrelse(&path);
- if (p)
- kunmap(bh_result->b_page);
if (result == IO_ERROR)
return -EIO;
/*
@@ -352,8 +350,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
}
pathrelse(&path);
- if (p)
- kunmap(bh_result->b_page);
return ret;
}
/* requested data are in direct item(s) */
@@ -363,8 +359,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
* when it is stored in direct item(s)
*/
pathrelse(&path);
- if (p)
- kunmap(bh_result->b_page);
return -ENOENT;
}
@@ -396,9 +390,7 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
* sure we need to. But, this means the item might move if
* kmap schedules
*/
- if (!p)
- p = (char *)kmap(bh_result->b_page);
-
+ p = (char *)kmap(bh_result->b_page);
p += offset;
memset(p, 0, inode->i_sb->s_blocksize);
do {
--
2.27.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-07-26 10:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-20 6:33 [PATCH] fs/reiserfs/inode: remove dead code in _get_block_create_0() zengjx95
2022-07-20 6:57 ` Al Viro
2022-07-20 8:30 zengjx95
2022-07-26 10:48 ` Jan Kara
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).