Re: [PATCH 06/12] ext2: drop posix acl handlers

[Jan Kara <jack@suse.cz>]

On Wed 25-01-23 12:28:51, Christian Brauner wrote:
> Last cycle we introduced a new posix acl api. Filesystems now only need
> to implement the inode operations for posix acls. The generic xattr
> handlers aren't used anymore by the vfs and will be completely removed.
> Keeping the handler around is confusing and gives the false impression
> that the xattr infrastructure of the vfs is used to interact with posix
> acls when it really isn't anymore.
> 
> For this to work we simply rework the ->listxattr() inode operation to
> not rely on the generix posix acl handlers anymore.
> 
> Cc: Jan Kara <jack@suse.com>
> Cc: <linux-ext4@vger.kernel.org>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Looks good to me. Feel free to add:

Acked-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext2/xattr.c | 60 +++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 641abfa4b718..86ba6a33349e 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -98,25 +98,9 @@ static struct buffer_head *ext2_xattr_cache_find(struct inode *,
>  static void ext2_xattr_rehash(struct ext2_xattr_header *,
>  			      struct ext2_xattr_entry *);
>  
> -static const struct xattr_handler *ext2_xattr_handler_map[] = {
> -	[EXT2_XATTR_INDEX_USER]		     = &ext2_xattr_user_handler,
> -#ifdef CONFIG_EXT2_FS_POSIX_ACL
> -	[EXT2_XATTR_INDEX_POSIX_ACL_ACCESS]  = &posix_acl_access_xattr_handler,
> -	[EXT2_XATTR_INDEX_POSIX_ACL_DEFAULT] = &posix_acl_default_xattr_handler,
> -#endif
> -	[EXT2_XATTR_INDEX_TRUSTED]	     = &ext2_xattr_trusted_handler,
> -#ifdef CONFIG_EXT2_FS_SECURITY
> -	[EXT2_XATTR_INDEX_SECURITY]	     = &ext2_xattr_security_handler,
> -#endif
> -};
> -
>  const struct xattr_handler *ext2_xattr_handlers[] = {
>  	&ext2_xattr_user_handler,
>  	&ext2_xattr_trusted_handler,
> -#ifdef CONFIG_EXT2_FS_POSIX_ACL
> -	&posix_acl_access_xattr_handler,
> -	&posix_acl_default_xattr_handler,
> -#endif
>  #ifdef CONFIG_EXT2_FS_SECURITY
>  	&ext2_xattr_security_handler,
>  #endif
> @@ -125,14 +109,41 @@ const struct xattr_handler *ext2_xattr_handlers[] = {
>  
>  #define EA_BLOCK_CACHE(inode)	(EXT2_SB(inode->i_sb)->s_ea_block_cache)
>  
> -static inline const struct xattr_handler *
> -ext2_xattr_handler(int name_index)
> +static const char *ext2_xattr_prefix(int xattr_index, struct dentry *dentry)
>  {
> +	const char *name = NULL;
>  	const struct xattr_handler *handler = NULL;
>  
> -	if (name_index > 0 && name_index < ARRAY_SIZE(ext2_xattr_handler_map))
> -		handler = ext2_xattr_handler_map[name_index];
> -	return handler;
> +	switch (xattr_index) {
> +	case EXT2_XATTR_INDEX_USER:
> +		handler = &ext2_xattr_user_handler;
> +		break;
> +	case EXT2_XATTR_INDEX_TRUSTED:
> +		handler = &ext2_xattr_trusted_handler;
> +		break;
> +#ifdef CONFIG_EXT2_FS_SECURITY
> +	case EXT2_XATTR_INDEX_SECURITY:
> +		handler = &ext2_xattr_security_handler;
> +		break;
> +#endif
> +#ifdef CONFIG_EXT2_FS_POSIX_ACL
> +	case EXT2_XATTR_INDEX_POSIX_ACL_ACCESS:
> +		if (posix_acl_dentry_list(dentry))
> +			name = XATTR_NAME_POSIX_ACL_ACCESS;
> +		break;
> +	case EXT2_XATTR_INDEX_POSIX_ACL_DEFAULT:
> +		if (posix_acl_dentry_list(dentry))
> +			name = XATTR_NAME_POSIX_ACL_DEFAULT;
> +		break;
> +#endif
> +	default:
> +		return NULL;
> +	}
> +
> +	if (xattr_dentry_list(handler, dentry))
> +		name = xattr_prefix(handler);
> +
> +	return name;
>  }
>  
>  static bool
> @@ -333,11 +344,10 @@ ext2_xattr_list(struct dentry *dentry, char *buffer, size_t buffer_size)
>  	/* list the attribute names */
>  	for (entry = FIRST_ENTRY(bh); !IS_LAST_ENTRY(entry);
>  	     entry = EXT2_XATTR_NEXT(entry)) {
> -		const struct xattr_handler *handler =
> -			ext2_xattr_handler(entry->e_name_index);
> +		const char *prefix;
>  
> -		if (handler && (!handler->list || handler->list(dentry))) {
> -			const char *prefix = handler->prefix ?: handler->name;
> +		prefix = ext2_xattr_prefix(entry->e_name_index, dentry);
> +		if (prefix) {
>  			size_t prefix_len = strlen(prefix);
>  			size_t size = prefix_len + entry->e_name_len + 1;
>  
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR