Re: [PATCH] ext4: Fix incorrect group count in ext4_fill_super error message

From: Josh Triplett <josh@joshtriplett.org>
To: Andreas Dilger <adilger@dilger.ca>
Cc: linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
	Theodore Ts'o <tytso@mit.edu>,
	Andreas Dilger <adilger.kernel@dilger.ca>
Subject: Re: [PATCH] ext4: Fix incorrect group count in ext4_fill_super error message
Date: Sat, 28 Mar 2020 16:55:47 -0700	[thread overview]
Message-ID: <20200328235547.GB27571@localhost> (raw)
In-Reply-To: <04E7F659-18B0-4CD2-8DE9-F69310BC3B06@dilger.ca>

On Sat, Mar 28, 2020 at 05:07:55PM -0600, Andreas Dilger wrote:
> On Mar 28, 2020, at 3:54 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > 
> > ext4_fill_super doublechecks the number of groups before mounting; if
> > that check fails, the resulting error message prints the group count
> > from the ext4_sb_info sbi, which hasn't been set yet. Print the freshly
> > computed group count instead (which at that point has just been computed
> > in "blocks_count").
> > 
> > Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> > Fixes: 4ec1102813798 ("ext4: Add sanity checks for the superblock before mounting the filesystem")
> 
> Modulo the compiler warning pointed out by kbuild test robot, I think the
> patch is correct, but was definitely confusing to read within the shown
> context, since "blocks_count" definitely doesn't seem to be "groups count"
> (it *is* the "groups count", but is just used as a temporary variable).

I agree that the code and patch read confusingly due to the (lack of)
context. The commit message attempted to explain that, but clearer code
seems preferable to confusing-but-explained code.

I sent a new version of this patch, which instead just moves the
assignment to sbi's group count earlier, so that the error message can
continue referencing it that way. (That also addresses the warning.)

- Josh Triplett