Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 17 May 2021 12:51:18 +0200
From:   David Sterba <dsterba@suse.cz>
To:     Khaled ROMDHANI <khaledromdhani216@gmail.com>
Cc:     clm@fb.com, josef@toxicpanda.com, dsterba@suse.com,
        linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] fs/btrfs: Fix uninitialized variable
Message-ID: <20210517105117.GL7604@twin.jikos.cz>
Reply-To: dsterba@suse.cz
Mail-Followup-To: dsterba@suse.cz,
        Khaled ROMDHANI <khaledromdhani216@gmail.com>, clm@fb.com,
        josef@toxicpanda.com, dsterba@suse.com, linux-btrfs@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
References: <20210501225046.9138-1-khaledromdhani216@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210501225046.9138-1-khaledromdhani216@gmail.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Precedence: bulk

On Sat, May 01, 2021 at 11:50:46PM +0100, Khaled ROMDHANI wrote:
> Fix the warning: variable 'zone' is used
> uninitialized whenever '?:' condition is true.
> 
> Fix that by preventing the code to reach
> the last assertion. If the variable 'mirror'
> is invalid, the assertion fails and we return
> immediately.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Khaled ROMDHANI <khaledromdhani216@gmail.com>

This took several rounds and none of them was close to what I'd consider
a proper fix, for something that's not really important. As Dan said,
smatch does understand the values passed from the callers and the
function is a static inline so the complete information is available. No
tricky analysis is required, so why does not coverity see that too?

We use assertions to namely catch programmer errors and API misuse,
anything that can happen at runtime or depends on input needs proper
checks and error handling. But for the super block copies, the constant
won't change so all we want is to catch the stupid errors.

> ---
>  fs/btrfs/zoned.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 8250ab3f0868..23da9d8dc184 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -145,7 +145,7 @@ static inline u32 sb_zone_number(int shift, int mirror)
>  	case 2: zone = 1ULL << (BTRFS_SB_LOG_SECOND_SHIFT - shift); break;
>  	default:
>  		ASSERT((u32)mirror < 3);
> -		break;
> +		return 0;

It's been pointed out that this does not apply on the current code but
on top of previous versions, so it's not making it easy for me to apply
the patch and do maybe some tweaks only.

I don't mind merging trivial patches, people can learn the process and
few iterations are not a big deal. What I also hope for is to get some
understanding of the code being changed and not just silencing some
tools' warnings.