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;
From:   Alex Dewar <alex.dewar90@gmail.com>
Date:   Sat, 10 Oct 2020 09:23:36 +0100
To:     Coly Li <colyli@suse.de>
Cc:     Rasmus Villemoes <rasmus.villemoes@prevas.dk>,
        Alex Dewar <alex.dewar90@gmail.com>,
        Kent Overstreet <kent.overstreet@gmail.com>,
        linux-bcache@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] bcache: Use #ifdef instead of boolean variable
Message-ID: <20201010082336.4435mnsxxmzqxq5y@medion>
References: <20201009183447.1611204-1-alex.dewar90@gmail.com>
 <b37a5dd2-437d-65e3-f432-a5247d09d94a@prevas.dk>
 <64db3f27-5904-8857-349e-e50d612932fb@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <64db3f27-5904-8857-349e-e50d612932fb@suse.de>
Precedence: bulk

On Sat, Oct 10, 2020 at 10:05:22AM +0800, Coly Li wrote:
> On 2020/10/10 07:00, Rasmus Villemoes wrote:
> > On 09/10/2020 20.34, Alex Dewar wrote:
> >> The variable async_registration is used to indicate whether or not
> >> CONFIG_BCACHE_ASYNC_REGISTRATION is enabled, triggering a (false)
> >> warning in Coverity about unreachable code. However, it seems clearer in
> >> this case just to use an #ifdef, so let's do that instead.
> >>
> >> Addresses-Coverity-ID: 1497746: Control flow issues (DEADCODE)
> > 
> > I think that coverity check needs to be ignored. The kernel is full of
> > things that are supposed to be eliminated by the compiler, but still
> > checked for valid syntax etc. Often it's even more hidden than this,
> > something like
> > 
> > // some header
> > #ifdef CONFIG_FOO
> > int foo(void);
> > #else
> > static inline int foo(void) { return 0; }
> > #endif
> > 
> > // code
> > 
> >   if (foo()) { ... // this goes away for CONFIG_FOO=n }
> > 
> >> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> >> ---
> >>  drivers/md/bcache/super.c | 40 +++++++++++++++++----------------------
> >>  1 file changed, 17 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> >> index 46a00134a36a..6d4127881c6a 100644
> >> --- a/drivers/md/bcache/super.c
> >> +++ b/drivers/md/bcache/super.c
> >> @@ -2504,11 +2504,6 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> >>  	struct cache_sb_disk *sb_disk;
> >>  	struct block_device *bdev;
> >>  	ssize_t ret;
> >> -	bool async_registration = false;
> >> -
> >> -#ifdef CONFIG_BCACHE_ASYNC_REGISTRATION
> >> -	async_registration = true;
> >> -#endif
> > 
> > If anything, this should simply be changed to
> > 
> >   bool async_registration = IS_ENABLED(CONFIG_BCACHE_ASYNC_REGISTRATION);
> > 
> > Rasmus
> 
> Hi Rasmus,
> 
> Yes, the above change might be better. But I don't suggest to spent
> effort on this.
> 
> Hi Alex,
> 
> Indeed the code in v5.9 is quite similar to what your patch makes, and I
> change it in this shape in v5.10 series. This piece of code may stay in
> kernel for 2 or 3 versions at most, the purpose is to make it convenient
> for people to test the async registration in production environment.
> Once the new async registration behavior is verified to not break any
> existing thing (which we don't know) it will be the (only) default
> behavior and the CONFIG_BCACHE_ASYNC_REGISTRATION check will be removed.
> 
> Thank you all for looking at this.
> 
> Coly Li

Hi Coly,

That sounds sensible. There's not much point in introducing needless
churn.

Best,
Alex