linux-ext4 - Re: [PATCH 2/2] ext4: clean up feature flag checking and usage

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: <20151003223609.GK3902@dastard>
Date:	Sun, 4 Oct 2015 09:36:09 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Andreas Dilger <adilger@...ger.ca>
Cc:	tytso@....edu, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 2/2] ext4: clean up feature flag checking and usage

On Sat, Oct 03, 2015 at 03:37:08AM -0600, Andreas Dilger wrote:
> Clean up the EXT4_{HAS,SET,CLEAR}_{,RO_,IN}COMPAT_FEATURE() flag
> checking to be easier to read, as well as safer from accidental
> coding mistakes.  Instead of passing the EXT4_FEATURE_*COMPAT_foo
> flag name to the macro, generate the EXT4_FEATURE_*COMPAT prefix
> within the macro, to ensure the flag name matches the check being
> done, since the numerical value might be for the wrong compat type.
> 
> Signed-off-by: Andreas Dilger <adilger@...ger.ca>
> ---
>  fs/ext4/balloc.c    |   17 ++---
>  fs/ext4/dir.c       |    7 +-
>  fs/ext4/ext4.h      |   70 ++++++++++---------
>  fs/ext4/ext4_jbd2.h |   26 +++----
>  fs/ext4/extents.c   |    4 +-
>  fs/ext4/ialloc.c    |    4 +-
>  fs/ext4/indirect.c  |    3 +-
>  fs/ext4/inline.c    |    3 +-
>  fs/ext4/inode.c     |   18 ++---
>  fs/ext4/ioctl.c     |   15 ++---
>  fs/ext4/migrate.c   |   13 ++--
>  fs/ext4/mmp.c       |    7 +-
>  fs/ext4/namei.c     |    8 +--
>  fs/ext4/resize.c    |   29 +++-----
>  fs/ext4/super.c     |  191 ++++++++++++++++++++++++---------------------------
>  fs/ext4/xattr.c     |    4 +-
>  16 files changed, 194 insertions(+), 225 deletions(-)
> 
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index cd6ea29..bc44979 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -213,7 +213,7 @@ static int ext4_init_block_bitmap(struct super_block *sb,
>  
>  	start = ext4_group_first_block_no(sb, block_group);
>  
> -	if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_FLEX_BG))
> +	if (EXT4_HAS_INCOMPAT_FEATURE(sb, FLEX_BG))

Rather than making it hard to use cscope/ctags to find the users of
these feature flags, wouldn't it be better to turn this around the
other way similar to the way XFS does this? i.e.:

 -	if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_FLEX_BG))
+	if (ext4_has_feature_flex_bg(sb))

static inline ext4_has_feature_flex_bg(struct super_block *sb)
{
	return EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_FLEX_BG);
}

That way the code doing the feature checks is clear and concise, is
much less shouty and it's not cluttered by whether features are in
the incompat, ro or compat matrices. As a bonus, cscope still works
just fine.

And then you can clean up/factor the macros and shorten the flags
knowing that they there are all in the one place and so doesn't
cause problems with code maintenance.

just my 2c worth...

Cheers,

Dave.

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