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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 27 Apr 2023 11:29:56 -0700
From:   Nathan Chancellor <nathan@...nel.org>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>, tytso@....edu
Cc:     adilger.kernel@...ger.ca, yanaijie@...wei.com,
        linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ext4: Fix unused iterator variable warnings

Hi Geert,

On Thu, Apr 27, 2023 at 02:36:10PM +0200, Geert Uytterhoeven wrote:
> On Thu, Apr 20, 2023 at 6:56 PM Nathan Chancellor <nathan@...nel.org> wrote:
> > When CONFIG_QUOTA is disabled, there are warnings around unused iterator
> > variables:
> >
> >   fs/ext4/super.c: In function 'ext4_put_super':
> >   fs/ext4/super.c:1262:13: error: unused variable 'i' [-Werror=unused-variable]
> >    1262 |         int i, err;
> >         |             ^
> >   fs/ext4/super.c: In function '__ext4_fill_super':
> >   fs/ext4/super.c:5200:22: error: unused variable 'i' [-Werror=unused-variable]
> >    5200 |         unsigned int i;
> >         |                      ^
> >   cc1: all warnings being treated as errors
> >
> > The kernel has updated to gnu11, allowing the variables to be declared
> > within the for loop. Do so to clear up the warnings.
> >
> > Fixes: dcbf87589d90 ("ext4: factor out ext4_flex_groups_free()")
> > Signed-off-by: Nathan Chancellor <nathan@...nel.org>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@...der.be>

Thank you for the review!

> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> 
> > @@ -1311,7 +1311,7 @@ static void ext4_put_super(struct super_block *sb)
> >         ext4_flex_groups_free(sbi);
> >         ext4_percpu_param_destroy(sbi);
> >  #ifdef CONFIG_QUOTA
> > -       for (i = 0; i < EXT4_MAXQUOTAS; i++)
> > +       for (int i = 0; i < EXT4_MAXQUOTAS; i++)
> 
> int
> 
> >                 kfree(get_qf_name(sb, sbi, i));
> >  #endif
> >
> 
> > @@ -5628,7 +5627,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> >  #endif
> >
> >  #ifdef CONFIG_QUOTA
> > -       for (i = 0; i < EXT4_MAXQUOTAS; i++)
> > +       for (unsigned int i = 0; i < EXT4_MAXQUOTAS; i++)
> 
> unsigned int
> 
> >                 kfree(get_qf_name(sb, sbi, i));
> >  #endif
> >         fscrypt_free_dummy_policy(&sbi->s_dummy_enc_policy);
> 
> I do see an opportunity to make this more consistent.
> get_qf_name() takes an int for the last parameter, but that should probably
> become unsigned int?

Yes, or I could have just changed the type of this variable to 'int', as
Arnd did in his version; I just chose to keep the existing type so this
was basically a "no functional change" patch.

https://lore.kernel.org/20230421070815.2260326-1-arnd@kernel.org/

I do not think it fundamentally matters, EXT4_MAXQUOTAS is defined as 3
so I do not think unsigned versus signed semantics matter much here :) I
can make that change in a v2 or separate change or we can just take
Arnd's patch, but this is now in mainline and there is another patch
trying to fix this warning so it would be good to get this dealt with
sooner rather than later...

https://lore.kernel.org/7ca8f790-c14e-6449-f3b5-4214d3fb1e61@googlemail.com/

Cheers,
Nathan

Powered by blists - more mailing lists