* [PATCH] fs/btrfs: Fix uninitialized variable @ 2021-05-01 22:50 Khaled ROMDHANI 2021-05-02 10:17 ` Christophe JAILLET ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Khaled ROMDHANI @ 2021-05-01 22:50 UTC (permalink / raw) To: clm, josef, dsterba Cc: Khaled ROMDHANI, linux-btrfs, linux-kernel, kernel-janitors 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> --- 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 -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/btrfs: Fix uninitialized variable 2021-05-01 22:50 [PATCH] fs/btrfs: Fix uninitialized variable Khaled ROMDHANI @ 2021-05-02 10:17 ` Christophe JAILLET 2021-05-03 8:49 ` Khaled Romdhani 2021-05-03 7:23 ` Dan Carpenter 2021-05-17 10:51 ` David Sterba 2 siblings, 1 reply; 8+ messages in thread From: Christophe JAILLET @ 2021-05-02 10:17 UTC (permalink / raw) To: Khaled ROMDHANI, clm, josef, dsterba Cc: linux-btrfs, linux-kernel, kernel-janitors Le 02/05/2021 à 00:50, Khaled ROMDHANI a écrit : > 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> > --- > 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); ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/btrfs: Fix uninitialized variable 2021-05-02 10:17 ` Christophe JAILLET @ 2021-05-03 8:49 ` Khaled Romdhani 0 siblings, 0 replies; 8+ messages in thread From: Khaled Romdhani @ 2021-05-03 8:49 UTC (permalink / raw) To: Christophe JAILLET Cc: clm, josef, dsterba, linux-btrfs, linux-kernel, kernel-janitors On Sun, May 02, 2021 at 12:17:51PM +0200, Christophe JAILLET wrote: > Le 02/05/2021 à 00:50, Khaled ROMDHANI a écrit : > > 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> > > --- > > 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); > Thank you for all of your comments. Yes, of course, they will help me. I will try to handle that more properly. Thanks again. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/btrfs: Fix uninitialized variable 2021-05-01 22:50 [PATCH] fs/btrfs: Fix uninitialized variable Khaled ROMDHANI 2021-05-02 10:17 ` Christophe JAILLET @ 2021-05-03 7:23 ` Dan Carpenter 2021-05-03 10:13 ` Khaled Romdhani 2021-05-17 10:51 ` David Sterba 2 siblings, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2021-05-03 7:23 UTC (permalink / raw) To: Khaled ROMDHANI Cc: clm, josef, dsterba, linux-btrfs, linux-kernel, kernel-janitors 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 is not how you send a v4 patch... v2 patches have to apply to the original code and not on top of the patched code. I sort of think you should find a different thing to work on. This code works fine as-is. Just leave it and try to find a real bug and fix that instead. regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/btrfs: Fix uninitialized variable 2021-05-03 7:23 ` Dan Carpenter @ 2021-05-03 10:13 ` Khaled Romdhani 2021-05-03 11:55 ` Dan Carpenter 0 siblings, 1 reply; 8+ messages in thread From: Khaled Romdhani @ 2021-05-03 10:13 UTC (permalink / raw) To: Dan Carpenter Cc: clm, josef, dsterba, linux-btrfs, linux-kernel, kernel-janitors On Mon, May 03, 2021 at 10:23:22AM +0300, Dan Carpenter wrote: > 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 is not how you send a v4 patch... v2 patches have to apply to the > original code and not on top of the patched code. > > I sort of think you should find a different thing to work on. This code > works fine as-is. Just leave it and try to find a real bug and fix that > instead. > > regards, > dan carpenter > Sorry, I was wrong and I shall send a proper V4. Yes, this code works fine just a warning caught by Coverity scan analysis. So the idea behind the patch is to fix the warning. To do that and as suggested by David Sterba, it would be rather to add a default case. So we fix the warning through the enhancement of the switch statement (some sort of 2*1). Yes, I will always try to find other bugs. It is a pleasure for me to do that. Thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/btrfs: Fix uninitialized variable 2021-05-03 10:13 ` Khaled Romdhani @ 2021-05-03 11:55 ` Dan Carpenter 2021-05-03 11:58 ` Colin Ian King 0 siblings, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2021-05-03 11:55 UTC (permalink / raw) To: Khaled Romdhani Cc: clm, josef, dsterba, linux-btrfs, linux-kernel, kernel-janitors On Mon, May 03, 2021 at 11:13:12AM +0100, Khaled Romdhani wrote: > On Mon, May 03, 2021 at 10:23:22AM +0300, Dan Carpenter wrote: > > 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 is not how you send a v4 patch... v2 patches have to apply to the > > original code and not on top of the patched code. > > > > I sort of think you should find a different thing to work on. This code > > works fine as-is. Just leave it and try to find a real bug and fix that > > instead. > > > > regards, > > dan carpenter > > > > Sorry, I was wrong and I shall send a proper V4. > > Yes, this code works fine just a warning caught by Coverity scan analysis. We're going to a lot of work to silence a static checker false positive. As a rule, I tell people to ignore the static checker when it is wrong. Btw, Smatch parses this code correctly and understands that the callers only pass valid values for "mirror". regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/btrfs: Fix uninitialized variable 2021-05-03 11:55 ` Dan Carpenter @ 2021-05-03 11:58 ` Colin Ian King 0 siblings, 0 replies; 8+ messages in thread From: Colin Ian King @ 2021-05-03 11:58 UTC (permalink / raw) To: Dan Carpenter, Khaled Romdhani Cc: clm, josef, dsterba, linux-btrfs, linux-kernel, kernel-janitors On 03/05/2021 12:55, Dan Carpenter wrote: > On Mon, May 03, 2021 at 11:13:12AM +0100, Khaled Romdhani wrote: >> On Mon, May 03, 2021 at 10:23:22AM +0300, Dan Carpenter wrote: >>> 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 is not how you send a v4 patch... v2 patches have to apply to the >>> original code and not on top of the patched code. >>> >>> I sort of think you should find a different thing to work on. This code >>> works fine as-is. Just leave it and try to find a real bug and fix that >>> instead. >>> >>> regards, >>> dan carpenter >>> >> >> Sorry, I was wrong and I shall send a proper V4. >> >> Yes, this code works fine just a warning caught by Coverity scan analysis. > > We're going to a lot of work to silence a static checker false positive. > As a rule, I tell people to ignore the static checker when it is wrong. > > Btw, Smatch parses this code correctly and understands that the callers > only pass valid values for "mirror". ..and Coverity does report a lot of false positives, so one needs to be really sure the issue is a genuine issue rather than a warning that can be ignore. Colin > > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/btrfs: Fix uninitialized variable 2021-05-01 22:50 [PATCH] fs/btrfs: Fix uninitialized variable Khaled ROMDHANI 2021-05-02 10:17 ` Christophe JAILLET 2021-05-03 7:23 ` Dan Carpenter @ 2021-05-17 10:51 ` David Sterba 2 siblings, 0 replies; 8+ messages in thread From: David Sterba @ 2021-05-17 10:51 UTC (permalink / raw) To: Khaled ROMDHANI Cc: clm, josef, dsterba, linux-btrfs, linux-kernel, kernel-janitors 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-05-17 10:54 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-01 22:50 [PATCH] fs/btrfs: Fix uninitialized variable Khaled ROMDHANI 2021-05-02 10:17 ` Christophe JAILLET 2021-05-03 8:49 ` Khaled Romdhani 2021-05-03 7:23 ` Dan Carpenter 2021-05-03 10:13 ` Khaled Romdhani 2021-05-03 11:55 ` Dan Carpenter 2021-05-03 11:58 ` Colin Ian King 2021-05-17 10:51 ` David Sterba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).