Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Apr 2023 11:29:56 -0700
From:   Nathan Chancellor <nathan@kernel.org>
To:     Geert Uytterhoeven <geert@linux-m68k.org>, tytso@mit.edu
Cc:     adilger.kernel@dilger.ca, yanaijie@huawei.com,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ext4: Fix unused iterator variable warnings
Message-ID: <20230427182956.GA2970@dev-arch.thelio-3990X>
References: <20230420-ext4-unused-variables-super-c-v1-1-138b6db6c21c@kernel.org>
 <CAMuHMdU0V=b1FLiT4UbNVTa42+5hFx3WJQD6gETwNYioSaSoag@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAMuHMdU0V=b1FLiT4UbNVTa42+5hFx3WJQD6gETwNYioSaSoag@mail.gmail.com>
Precedence: bulk

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@kernel.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@kernel.org>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.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