Re: [PATCH -V1 22/22] ext4: Add Ext4 compat richacl feature flag

[Andreas Dilger]

On May 1, 2014, at 9:48 AM, Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx> wrote:

> Andreas Dilger <adilger@xxxxxxxxx> writes:
> 
>> On Apr 27, 2014, at 10:14 AM, Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx> wrote:
>>> This feature flag can be used to enable richacl on
>>> the file system. Once enabled the "acl" mount option
>>> will enable richacl instead of posix acl
>> 
>>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>>> index 6f9e6fadac04..2a0221652d79 100644
>>> --- a/fs/ext4/super.c
>>> +++ b/fs/ext4/super.c
>>> @@ -1274,6 +1274,30 @@ static ext4_fsblk_t get_sb_block(void **data)
>>> 	return sb_block;
>>> }
>>> 
>>> +static void enable_acl(struct super_block *sb)
>>> +{
>>> +#if !defined(CONFIG_EXT4_FS_POSIX_ACL) && !defined(CONFIG_EXT4_FS_RICHACL)
>>> +	return;
>>> +#endif
>>> +	if (EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_RICHACL)) {
>>> +		sb->s_flags |= MS_RICHACL;
>>> +		sb->s_flags &= ~MS_POSIXACL;
>>> +	} else {
>>> +		sb->s_flags |= MS_POSIXACL;
>>> +		sb->s_flags &= ~MS_RICHACL;
>>> +	}
>> 
>> This should put the #ifdef around the code that is being enabled/disabled,
>> otherwise it just becomes dead code:
>> 
>> static int enable_acl(struct super_block *sb)
>> {
>> 	if (EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_RICHACL)) {
>> #if defined(CONFIG_EXT4_FS_RICHACL)
>> 		sb->s_flags |= MS_RICHACL;
>> 		sb->s_flags &= ~MS_POSIXACL;
>> #else
>> 		return -EOPNOTSUPP;
>> #endif
>> 	} else {
>> #if defined(CONFIG_EXT4_FS_POSIX_ACL)
>> 		sb->s_flags |= MS_POSIXACL;
>> 		sb->s_flags &= ~MS_RICHACL;
>> #else
>> 		return -EOPNOTSUPP;
>> #endif
>> 	}
>> 	return 0;
>> }
> 
> That is too much #ifdef with no real benefit ?

The benefit is that if neither CONFIG_EXT4_FS_RICHACL nor CONFIG_EXT4_FS_POSIX_ACL are defined there isn't unreachable code
after "return" at the start of the function.  Some static code
analysis tools will complain about this.

Cheers, Andreas

Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail