linux-kernel - Re: [PATCH] ext4: Fix unused iterator variable warnings

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230427182956.GA2970@dev-arch.thelio-3990X>
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