linux-kernel - Re: [PATCH] bcache: Use #ifdef instead of boolean variable

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]

Date:   Sat, 10 Oct 2020 09:23:36 +0100
From:   Alex Dewar <alex.dewar90@...il.com>
To:     Coly Li <colyli@...e.de>
Cc:     Rasmus Villemoes <rasmus.villemoes@...vas.dk>,
        Alex Dewar <alex.dewar90@...il.com>,
        Kent Overstreet <kent.overstreet@...il.com>,
        linux-bcache@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] bcache: Use #ifdef instead of boolean variable

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@...il.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