ext4: Ensure Inode flags consistency are checked in build time [V2]

Flags being used by atomic operations in inode flags (e.g.
ext4_test_inode_flag(), should be consistent with that actually stored in
inodes, i.e.: EXT4_XXX_FL.

It ensures that this consistency is checked at build-time, not at run-time.

Currently, the flags consistency are being checked at run-time, but, there is no
real reason to not do a build-time check instead of a run-time check. The code
is comparing macro defined values with enum type variables, where both are
constants, so, there is no problem in comparing constants at build-time.

enum variables are treated as constants by the C compiler, according to the C99
specs (see www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf sec. 6.2.5, item
16), so, there is no real problem in comparing an enumeration type at build time

CC'ing Sergio who helped me to achieve this conclusion, in case there is
something else we need to discuss.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/ext4/ext4.h  | 29 +++++++++++++----------------
 fs/ext4/super.c |  1 +
 2 files changed, 14 insertions(+), 16 deletions(-)

CC'ing Sergio as I didn't when the patch was sent

On Mon, Dec 03, 2012 at 03:32:56PM -0200, Carlos Maiolino wrote:
> Flags being used by atomic operations in inode flags (e.g.
> ext4_test_inode_flag(), should be consistent with that actually stored in
> inodes, i.e.: EXT4_XXX_FL.
> 
> It ensures that this consistency is checked at build-time, not at run-time.
> 
> Currently, the flags consistency are being checked at run-time, but, there is no
> real reason to not do a build-time check instead of a run-time check. The code
> is comparing macro defined values with enum type variables, where both are
> constants, so, there is no problem in comparing constants at build-time.
> 
> enum variables are treated as constants by the C compiler, according to the C99
> specs (see www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf sec. 6.2.5, item
> 16), so, there is no real problem in comparing an enumeration type at build time
> 
> CC'ing Sergio who helped me to achieve this conclusion, in case there is
> something else we need to discuss.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/ext4/ext4.h  | 29 +++++++++++++----------------
>  fs/ext4/super.c |  1 +
>  2 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 3c20de1..4ac0523 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -451,25 +451,22 @@ enum {
>  	EXT4_INODE_RESERVED	= 31,	/* reserved for ext4 lib */
>  };
>  
> -#define TEST_FLAG_VALUE(FLAG) (EXT4_##FLAG##_FL == (1 << EXT4_INODE_##FLAG))
> -#define CHECK_FLAG_VALUE(FLAG) if (!TEST_FLAG_VALUE(FLAG)) { \
> -	printk(KERN_EMERG "EXT4 flag fail: " #FLAG ": %d %d\n", \
> -		EXT4_##FLAG##_FL, EXT4_INODE_##FLAG); BUG_ON(1); }
> -
> -/*
> - * Since it's pretty easy to mix up bit numbers and hex values, and we
> - * can't do a compile-time test for ENUM values, we use a run-time
> - * test to make sure that EXT4_XXX_FL is consistent with respect to
> - * EXT4_INODE_XXX.  If all is well the printk and BUG_ON will all drop
> - * out so it won't cost any extra space in the compiled kernel image.
> - * But it's important that these values are the same, since we are
> - * using EXT4_INODE_XXX to test for the flag values, but EXT4_XX_FL
> - * must be consistent with the values of FS_XXX_FL defined in
> - * include/linux/fs.h and the on-disk values found in ext2, ext3, and
> - * ext4 filesystems, and of course the values defined in e2fsprogs.
> +/*
> + * Since it's pretty easy to mix up bit numbers and hex values, we use a
> + * build-time check to make sure that EXT4_XXX_FL is consistent with respect to
> + * EXT4_INODE_XXX. If all is well, the macros will be dropped, so, it won't cost
> + * any extra space in the compiled kernel image, otherwise, the build will fail.
> + * It's important that these values are the same, since we are using
> + * EXT4_INODE_XXX to test for flag values, but EXT4_XXX_FL must be consistent
> + * with the values of FS_XXX_FL defined in include/linux/fs.h and the on-disk
> + * values found in ext2, ext3 and ext4 filesystems, and of course the values
> + * defined in e2fsprogs.
>   *
>   * It's not paranoia if the Murphy's Law really *is* out to get you.  :-)
>   */
> +#define TEST_FLAG_VALUE(FLAG) (EXT4_##FLAG##_FL == (1 << EXT4_INODE_##FLAG))
> +#define CHECK_FLAG_VALUE(FLAG) BUILD_BUG_ON(!TEST_FLAG_VALUE(FLAG))
> +
>  static inline void ext4_check_flag_values(void)
>  {
>  	CHECK_FLAG_VALUE(SECRM);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 80928f7..e6f6f8b 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5282,6 +5282,7 @@ static int __init ext4_init_fs(void)
>  	ext4_li_info = NULL;
>  	mutex_init(&ext4_li_mtx);
>  
> +	/* Build-time check for flags consistency */
>  	ext4_check_flag_values();
>  
>  	for (i = 0; i < EXT4_WQ_HASH_SZ; i++) {
> -- 
> 1.7.11.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Dec 03, 2012 at 03:32:56PM -0200, Carlos Maiolino wrote:
> Flags being used by atomic operations in inode flags (e.g.
> ext4_test_inode_flag(), should be consistent with that actually stored in
> inodes, i.e.: EXT4_XXX_FL.
> 
> It ensures that this consistency is checked at build-time, not at run-time.
> 
> Currently, the flags consistency are being checked at run-time, but, there is no
> real reason to not do a build-time check instead of a run-time check. The code
> is comparing macro defined values with enum type variables, where both are
> constants, so, there is no problem in comparing constants at build-time.
> 
> enum variables are treated as constants by the C compiler, according to the C99
> specs (see www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf sec. 6.2.5, item
> 16), so, there is no real problem in comparing an enumeration type at build time
> 
> CC'ing Sergio who helped me to achieve this conclusion, in case there is
> something else we need to discuss.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>

Thanks, applied.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html