Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 20 Jul 2022 07:57:45 +0100
From:   Al Viro <viro@zeniv.linux.org.uk>
To:     zengjx95@gmail.com
Cc:     reiserfs-devel@vger.kernel.org, willy@infradead.org,
        jlayton@kernel.org, damien.lemoal@opensource.wdc.com, jack@suse.cz,
        edward.shishkin@gmail.com, linux-kernel@vger.kernel.org,
        Zeng Jingxiang <linuszeng@tencent.com>
Subject: Re: [PATCH] fs/reiserfs/inode: remove dead code in
 _get_block_create_0()
Message-ID: <YtenaY4hDx7J746Y@ZenIV>
References: <20220720063310.992149-1-zengjx95@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220720063310.992149-1-zengjx95@gmail.com>
Sender: Al Viro <viro@ftp.linux.org.uk>
Precedence: bulk

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
>