All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: Christian Brauner <brauner@kernel.org>
Cc: linux-fsdevel@vger.kernel.org, Christoph Hellwig <hch@lst.de>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Seth Forshee <sforshee@kernel.org>,
	linux-erofs@lists.ozlabs.org
Subject: Re: [PATCH 05/12] erofs: drop posix acl handlers
Date: Mon, 30 Jan 2023 07:43:29 +0100	[thread overview]
Message-ID: <20230130064329.GF31145@lst.de> (raw)
In-Reply-To: <20230125-fs-acl-remove-generic-xattr-handlers-v1-5-6cf155b492b6@kernel.org>

This review is not for erofs specifically, but for all file systems using
the same scheme.

> +static const char *erofs_xattr_prefix(int xattr_index, struct dentry *dentry)
> +{
> +	const char *name = NULL;
> +	const struct xattr_handler *handler = NULL;
> +
> +	switch (xattr_index) {
> +	case EROFS_XATTR_INDEX_USER:
> +		handler = &erofs_xattr_user_handler;
> +		break;
> +	case EROFS_XATTR_INDEX_TRUSTED:
> +		handler = &erofs_xattr_trusted_handler;
> +		break;
> +#ifdef CONFIG_EROFS_FS_SECURITY
> +	case EROFS_XATTR_INDEX_SECURITY:
> +		handler = &erofs_xattr_security_handler;
> +		break;
> +#endif
> +#ifdef CONFIG_EROFS_FS_POSIX_ACL
> +	case EROFS_XATTR_INDEX_POSIX_ACL_ACCESS:
> +		if (posix_acl_dentry_list(dentry))
> +			name = XATTR_NAME_POSIX_ACL_ACCESS;
> +		break;
> +	case EROFS_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);

I'm not a huge fan of all this duplicate logic in the file systems
that is more verbose and a bit confusing.  Until we remove the
xattr handlers entirely, I wonder if we just need to keep a
special ->list for posix xattrs, just to be able to keep the
old logic in all these file system.  That is a ->list that
works for xattr_dentry_list, but never actually lists anything.

That would remove all this boiler plate for now without minimal
core additions.  Eventually we can hopefully remove first ->list
and then the xattr handlers entirely, but until then this seems
like a step backwards.

WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@lst.de>
To: Christian Brauner <brauner@kernel.org>
Cc: linux-fsdevel@vger.kernel.org, Seth Forshee <sforshee@kernel.org>,
	linux-erofs@lists.ozlabs.org, Christoph Hellwig <hch@lst.de>,
	Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH 05/12] erofs: drop posix acl handlers
Date: Mon, 30 Jan 2023 07:43:29 +0100	[thread overview]
Message-ID: <20230130064329.GF31145@lst.de> (raw)
In-Reply-To: <20230125-fs-acl-remove-generic-xattr-handlers-v1-5-6cf155b492b6@kernel.org>

This review is not for erofs specifically, but for all file systems using
the same scheme.

> +static const char *erofs_xattr_prefix(int xattr_index, struct dentry *dentry)
> +{
> +	const char *name = NULL;
> +	const struct xattr_handler *handler = NULL;
> +
> +	switch (xattr_index) {
> +	case EROFS_XATTR_INDEX_USER:
> +		handler = &erofs_xattr_user_handler;
> +		break;
> +	case EROFS_XATTR_INDEX_TRUSTED:
> +		handler = &erofs_xattr_trusted_handler;
> +		break;
> +#ifdef CONFIG_EROFS_FS_SECURITY
> +	case EROFS_XATTR_INDEX_SECURITY:
> +		handler = &erofs_xattr_security_handler;
> +		break;
> +#endif
> +#ifdef CONFIG_EROFS_FS_POSIX_ACL
> +	case EROFS_XATTR_INDEX_POSIX_ACL_ACCESS:
> +		if (posix_acl_dentry_list(dentry))
> +			name = XATTR_NAME_POSIX_ACL_ACCESS;
> +		break;
> +	case EROFS_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);

I'm not a huge fan of all this duplicate logic in the file systems
that is more verbose and a bit confusing.  Until we remove the
xattr handlers entirely, I wonder if we just need to keep a
special ->list for posix xattrs, just to be able to keep the
old logic in all these file system.  That is a ->list that
works for xattr_dentry_list, but never actually lists anything.

That would remove all this boiler plate for now without minimal
core additions.  Eventually we can hopefully remove first ->list
and then the xattr handlers entirely, but until then this seems
like a step backwards.

  reply	other threads:[~2023-01-30  6:43 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-25 11:28 [f2fs-dev] [PATCH 00/12] acl: remove remaining posix acl handlers Christian Brauner
2023-01-25 11:28 ` Christian Brauner
2023-01-25 11:28 ` [Ocfs2-devel] " Christian Brauner via Ocfs2-devel
2023-01-25 11:28 ` Christian Brauner
2023-01-25 11:28 ` Christian Brauner
2023-01-25 11:28 ` Christian Brauner
2023-01-25 11:28 ` [PATCH 01/12] xattr: simplify listxattr helpers Christian Brauner
2023-01-30  6:35   ` Christoph Hellwig
2023-01-25 11:28 ` [PATCH 02/12] xattr, posix acl: add " Christian Brauner
2023-01-30  6:37   ` Christoph Hellwig
2023-01-25 11:28 ` [PATCH 03/12] xattr: remove unused argument Christian Brauner
2023-01-30  6:37   ` Christoph Hellwig
2023-01-25 11:28 ` [PATCH 04/12] fs: drop unused posix acl handlers Christian Brauner
2023-01-30  6:38   ` Christoph Hellwig
2023-01-25 11:28 ` [PATCH 05/12] erofs: drop " Christian Brauner
2023-01-25 11:28   ` Christian Brauner
2023-01-30  6:43   ` Christoph Hellwig [this message]
2023-01-30  6:43     ` Christoph Hellwig
2023-01-30  9:00     ` Christian Brauner
2023-01-30  9:00       ` Christian Brauner
2023-01-30  9:11       ` Christoph Hellwig
2023-01-30  9:11         ` Christoph Hellwig
2023-01-25 11:28 ` [PATCH 06/12] ext2: " Christian Brauner
2023-01-25 13:03   ` Jan Kara
2023-01-25 11:28 ` [PATCH 07/12] ext4: " Christian Brauner
2023-01-25 11:28 ` [f2fs-dev] [PATCH 08/12] f2fs: " Christian Brauner
2023-01-25 11:28   ` Christian Brauner
2023-01-25 11:28 ` [PATCH 09/12] jffs2: " Christian Brauner
2023-01-25 11:28   ` Christian Brauner
2023-01-25 11:28 ` [PATCH 10/12] ocfs2: " Christian Brauner
2023-01-25 11:28   ` [Ocfs2-devel] " Christian Brauner via Ocfs2-devel
2023-01-25 11:28 ` [PATCH 11/12] reiserfs: " Christian Brauner
2023-01-25 11:28 ` [PATCH 12/12] acl: remove " Christian Brauner
2023-01-31 12:04   ` kernel test robot
2023-01-30  9:10 ` [PATCH 00/12] acl: remove remaining " Christian Brauner
2023-01-30  9:10   ` Christian Brauner
2023-01-30  9:10   ` [f2fs-dev] " Christian Brauner
2023-01-30  9:10   ` Christian Brauner
2023-01-30  9:10   ` [Ocfs2-devel] " Christian Brauner via Ocfs2-devel
2023-01-30  9:16   ` Christoph Hellwig
2023-01-30  9:16     ` Christoph Hellwig
2023-01-30  9:16     ` Christoph Hellwig
2023-01-30  9:16     ` [f2fs-dev] " Christoph Hellwig
2023-01-30  9:16     ` [Ocfs2-devel] " Christoph Hellwig via Ocfs2-devel
2023-01-30 10:23     ` Christian Brauner
2023-01-30 10:23       ` Christian Brauner
2023-01-30 10:23       ` Christian Brauner
2023-01-30 10:23       ` [f2fs-dev] " Christian Brauner
2023-01-30 10:23       ` [Ocfs2-devel] " Christian Brauner via Ocfs2-devel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230130064329.GF31145@lst.de \
    --to=hch@lst.de \
    --cc=brauner@kernel.org \
    --cc=linux-erofs@lists.ozlabs.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=sforshee@kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.