[PATCH-next] fs/btrfs: Fix uninitialized variable

[PATCH-next] fs/btrfs: Fix uninitialized variable

[Khaled ROMDHANI]

The variable 'zone' is uninitialized which
introduce some build warning.

It is not always set or overwritten within
the function. So explicitly initialize it.

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 432509f4b3ac..42f99b25127f 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -136,7 +136,7 @@ static int sb_write_pointer(struct block_device *bdev, struct blk_zone *zones,
  */
 static inline u32 sb_zone_number(int shift, int mirror)
 {
-	u64 zone;
+	u64 zone = 0;
 
 	ASSERT(mirror < BTRFS_SUPER_MIRROR_MAX);
 	switch (mirror) {

base-commit: c05b2a58c9ed11bd753f1e64695bd89da715fbaa
-- 
2.17.1

Re: [PATCH-next] fs/btrfs: Fix uninitialized variable

[David Sterba]

On Fri, Apr 23, 2021 at 01:42:01PM +0100, Khaled ROMDHANI wrote:
> The variable 'zone' is uninitialized which
> introduce some build warning.
> 
> It is not always set or overwritten within
> the function. So explicitly initialize it.
> 
> 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 432509f4b3ac..42f99b25127f 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -136,7 +136,7 @@ static int sb_write_pointer(struct block_device *bdev, struct blk_zone *zones,
>   */
>  static inline u32 sb_zone_number(int shift, int mirror)
>  {
> -	u64 zone;
> +	u64 zone = 0;

This is exactly same as v1
https://lore.kernel.org/linux-btrfs/20210413130604.11487-1-khaledromdhani216@gmail.com/

It does fix the build warning but does not make sense in the code
because it would would silently let mirror == 4 pass. I think there was
enough feedback under the previous posts how to fix that properly.

Re: [PATCH-next] fs/btrfs: Fix uninitialized variable

[Khaled Romdhani]

On Mon, Apr 26, 2021 at 10:19:29PM +0200, David Sterba wrote:
> On Fri, Apr 23, 2021 at 01:42:01PM +0100, Khaled ROMDHANI wrote:
> > The variable 'zone' is uninitialized which
> > introduce some build warning.
> > 
> > It is not always set or overwritten within
> > the function. So explicitly initialize it.
> > 
> > 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 432509f4b3ac..42f99b25127f 100644
> > --- a/fs/btrfs/zoned.c
> > +++ b/fs/btrfs/zoned.c
> > @@ -136,7 +136,7 @@ static int sb_write_pointer(struct block_device *bdev, struct blk_zone *zones,
> >   */
> >  static inline u32 sb_zone_number(int shift, int mirror)
> >  {
> > -	u64 zone;
> > +	u64 zone = 0;
> 
> This is exactly same as v1
> https://lore.kernel.org/linux-btrfs/20210413130604.11487-1-khaledromdhani216@gmail.com/
> 
> It does fix the build warning but does not make sense in the code
> because it would would silently let mirror == 4 pass. I think there was
> enough feedback under the previous posts how to fix that properly.

Yes, it fixes the build warning. I implemented a tiny test
program before sending the patch. In which I explicitly
set 'm=5' to check the change results:

[356843.099365] assertion failed: z, in /home/khaled/fs_btrfs/test3.c:30

From the above output message, I think that it catchs the assertion.
Sorry, but let me know if I am missing something.

I absolutly agree with you regarding the waste of memory
when doing some pointless initializations. I will, come
back with a V2.

Thanks.

[PATCH-next] fs/btrfs: Fix uninitialized variable

[Khaled ROMDHANI]

The variable zone is not initialized. It
may causes a failed assertion.

Addresses-Coverity: ("Uninitialized variables")

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 eeb3ebe11d7a..ee15ab8dccb5 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -136,7 +136,7 @@ static int sb_write_pointer(struct block_device *bdev, struct blk_zone *zones,
  */
 static inline u32 sb_zone_number(int shift, int mirror)
 {
-	u64 zone;
+	u64 zone = 0;
 
 	ASSERT(mirror < BTRFS_SUPER_MIRROR_MAX);
 	switch (mirror) {
-- 
2.17.1

Re: [PATCH-next] fs/btrfs: Fix uninitialized variable

[Boris Burkov]

On Tue, Apr 13, 2021 at 02:06:04PM +0100, Khaled ROMDHANI wrote:
> The variable zone is not initialized. It
> may causes a failed assertion.
> 
> Addresses-Coverity: ("Uninitialized variables")
> 
> Signed-off-by: Khaled ROMDHANI <khaledromdhani216@gmail.com>

Reviewed-by: Boris Burkov <boris@bur.io>

> ---
>  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 eeb3ebe11d7a..ee15ab8dccb5 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -136,7 +136,7 @@ static int sb_write_pointer(struct block_device *bdev, struct blk_zone *zones,
>   */
>  static inline u32 sb_zone_number(int shift, int mirror)
>  {
> -	u64 zone;
> +	u64 zone = 0;
>  
>  	ASSERT(mirror < BTRFS_SUPER_MIRROR_MAX);

Thanks for the fix.

I assume this was dug up by coverity static analysis rather than hitting
it in a live system?

Since there is already an assert for the pre-condition 'mirror < max',
I feel like it would make sense to also add one for mirror > 0.

>  	switch (mirror) {
> -- 
> 2.17.1
> 

Re: [PATCH-next] fs/btrfs: Fix uninitialized variable

[David Sterba]

On Tue, Apr 13, 2021 at 02:06:04PM +0100, Khaled ROMDHANI wrote:
> The variable zone is not initialized. It
> may causes a failed assertion.

Failed assertion means the 2nd one checking that the result still fits
to 32bit type. That would mean that none of the cases were hit, but all
callers pass valid values.

It would be better to add a default: case to catch that explicitly,
though hitting that is considered 'will not happen'.

Re: [PATCH-next] fs/btrfs: Fix uninitialized variable

[Khaled Romdhani]

On Fri, Apr 16, 2021 at 07:32:03PM +0200, David Sterba wrote:
> On Tue, Apr 13, 2021 at 02:06:04PM +0100, Khaled ROMDHANI wrote:
> > The variable zone is not initialized. It
> > may causes a failed assertion.
> 
> Failed assertion means the 2nd one checking that the result still fits
> to 32bit type. That would mean that none of the cases were hit, but all
> callers pass valid values.
> 
> It would be better to add a default: case to catch that explicitly,
> though hitting that is considered 'will not happen'.

Yes. I will send a V2.