Re: [PATCH] fs/btrfs: Fix uninitialized variable
From: Christophe JAILLET <hidden>
Date: 2021-05-02 10:18:03
Also in:
kernel-janitors, lkml
Subsystem:
btrfs file system, filesystems (vfs and infrastructure), the rest · Maintainers:
Chris Mason, David Sterba, Alexander Viro, Christian Brauner, Linus Torvalds
Le 02/05/2021 à 00:50, Khaled ROMDHANI a écrit :
quoted hunk ↗ jump to hunk
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 <redacted> Signed-off-by: Khaled ROMDHANI <redacted> --- 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; } ASSERT(zone <= U32_MAX);base-commit: b5c294aac8a6164ddf38bfbdd1776091b4a1eeba
Hi, just a few comments. If I understand correctly, what you try to do is to silence a compiler warning if no case branch is taken. First, all your proposals are based on the previous one. I find it hard to follow because we don't easily see what are the differences since the beginning. The "base-commit" at the bottom of your mail, is related to your own local tree, I guess. It can't be used by any-one. My understanding it that a patch, should it be v2, v3..., must apply to the current tree. (In my case, it is the latest linux-next) This is not the case here and you have to apply each step to see the final result. Should this version be fine, a maintainer wouldn't be able to apply it as-is. You also try to take into account previous comments to check for incorrect negative values for minor and catch (the can't happen today) cases, should BTRFS_SUPER_MIRROR_MAX change and this function remain the same. So, why hard-coding '3'? The reason of magic numbers are hard to remember. You should avoid them or add a comment about it. My own personal variation would be something like the code below (untested). Hope this helps. CJ
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 70b23a0d03b1..75fe5f001d8b 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c@@ -138,11 +138,14 @@ static inline u32 sb_zone_number(int shift, int mirror)
{
u64 zone;
- ASSERT(mirror < BTRFS_SUPER_MIRROR_MAX);
+ ASSERT(mirror >= 0 && mirror < BTRFS_SUPER_MIRROR_MAX);
switch (mirror) {
case 0: zone = 0; break;
case 1: zone = 1ULL << (BTRFS_SB_LOG_FIRST_SHIFT - shift); break;
case 2: zone = 1ULL << (BTRFS_SB_LOG_SECOND_SHIFT - shift); break;
+ default:
+ ASSERT(! "mirror < BTRFS_SUPER_MIRROR_MAX but not handled above.");
+ return 0;
}
ASSERT(zone <= U32_MAX);