Re: [PATCH 05/12] erofs: drop posix acl handlers

From: Christoph Hellwig <hch@lst.de>
To: Christian Brauner <brauner@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>,
	linux-fsdevel@vger.kernel.org, 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 10:11:32 +0100	[thread overview]
Message-ID: <20230130091132.GA5178@lst.de> (raw)
In-Reply-To: <20230130090008.zg5ddfanrgeommrv@wittgenstein>

On Mon, Jan 30, 2023 at 10:00:08AM +0100, Christian Brauner wrote:
> > > +	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
> 
> Yeah, it hasn't been my favorite part about this either.
> But note how the few filesystems that receive that change use the same
> logic by indexing an array and retrieving the handler and then clumsily
> open-coding the same check that is now moved into xattr_dentry_list().

At least it allows for an array lookup.  And of course switching
to xattr_dentry_list instead of open coding it is always a good idea.

> If we want the exact same logic to be followed as today then we need to
> keep the dummy struct posix_acl_{access,default}_xattr_handler around.
> I tried to avoid that for the first version because it felt a bit
> disappointing but we can live with this. This way there's zero code changes
> required for filesystems that use legacy array-based handler-indexing.

Yes, I'd just leave those as-is using the handlers.  I don't really
like the result, but the changes in the series doesn't really look
better and causes extra churn.  In the long run struct xattr_handler
needs to go away and we'll need separate handlers for each type
of xattrs, but that's going to take a while.  Do you know where the
capabilities conversion is standing?

> But we should probably still tweak this so that all these filesystems don't
> open-code the !h || (h->list && !h->list(dentry) check like they do now. So
> something like what I did below at [1]. Thoughts?

Yes, that part is useful.

> +static inline const char *erofs_xattr_prefix(unsigned int idx, struct dentry *dentry)

Overly long line here, though.