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

[Coly Li]

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@xxxxxxxxx>
>> ---
>>  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