linux-kernel - Re: [PATCH v2] btrfs: Remove redundant initialization of 'to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5938551d-a083-a62c-4ab3-bc29fc62b1df@gmx.com>
Date:   Mon, 17 May 2021 18:13:50 +0800
From:   Qu Wenruo <quwenruo.btrfs@....com>
To:     Yang Li <yang.lee@...ux.alibaba.com>, clm@...com
Cc:     josef@...icpanda.com, dsterba@...e.com, nathan@...nel.org,
        ndesaulniers@...gle.com, linux-btrfs@...r.kernel.org,
        lukas.bulwahn@...il.com, linux-kernel@...r.kernel.org,
        clang-built-linux@...glegroups.com
Subject: Re: [PATCH v2] btrfs: Remove redundant initialization of 'to_add'

On 2021/5/17 下午5:46, Yang Li wrote:
> Variable 'to_add' is being initialized however this value is never
> read as 'to_add' is assigned a new value in if statement. Remove the
> redundant assignment. At the same time, move its declaration into the
> if statement, because the variable is not used elsewhere.
>
> Clean up clang warning:
>
> fs/btrfs/extent-tree.c:2774:8: warning: Value stored to 'to_add' during
> its initialization is never read [clang-analyzer-deadcode.DeadStores]

Personally speaking, compiler should be able to optimize out such
problem, nothing really worthy bothering.

Especially considering these "fixes" just randomly pop up, distracting
the reviewers' time.

If you really believe these "fixes" are really worthy (not to just
fulfill the stupid KPI), please at least pack them into a larger
patchset (but keep the separate patches), not just sending one when you
find one.

>
> Reported-by: Abaci Robot <abaci@...ux.alibaba.com>

I know some maintainers are already very upset about the bot, although
in your case it reduces a lifespan of a variable, thus it's marginally
acceptable, but under other cases, it doesn't really help much.

If such fixes come from indie developers, I'm pretty fine or even happy
to help them to start more contribution.

But a sponsored bot just repeating clang static analyzer

Trust me, no maintainer will be happy with that, and you're destroying
the reputation of your company (if the reputation hasn't been destoryed
already).

> Signed-off-by: Yang Li <yang.lee@...ux.alibaba.com>
> ---
>
> Change in v2:
> --According to Lukas's suggestion, combine the declaration and assignment of
>    variable 'to_add' into one line, just as "u64 to_add = min(len, ...);"
>    https://lore.kernel.org/patchwork/patch/1428697/
>
>   fs/btrfs/extent-tree.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index f1d15b6..13ac978 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2774,11 +2774,9 @@ static int unpin_extent_range(struct btrfs_fs_info *fs_info,
>   		spin_unlock(&cache->lock);
>   		if (!readonly && return_free_space &&
>   		    global_rsv->space_info == space_info) {
> -			u64 to_add = len;
> -
>   			spin_lock(&global_rsv->lock);
>   			if (!global_rsv->full) {
> -				to_add = min(len, global_rsv->size -
> +				u64 to_add = min(len, global_rsv->size -
>   					     global_rsv->reserved);

Have you ever wondered why "global_rsv" is not indented by tab only, but
with extra spaces?

It's supposed to be aligned with "len".

Thanks,
Qu
>   				global_rsv->reserved += to_add;
>   				btrfs_space_info_update_bytes_may_use(fs_info,
>