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  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]
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

Powered by blists - more mailing lists