On Mon, Jan 30, 2023 at 07:43:29AM +0100, Christoph Hellwig wrote:
> 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
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().
> 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.
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.
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?
[1]:
diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index a62fb8a3318a..fd83bbc4b98a 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -496,13 +496,9 @@ static int xattr_entrylist(struct xattr_iter *_it,
unsigned int prefix_len;
const char *prefix;
- const struct xattr_handler *h =
- erofs_xattr_handler(entry->e_name_index);
-
- if (!h || (h->list && !h->list(it->dentry)))
+ prefix = erofs_xattr_prefix(entry->e_name_index, it->dentry);
+ if (!prefix)
return 1;
-
- prefix = xattr_prefix(h);
prefix_len = strlen(prefix);
if (!it->buffer) {
diff --git a/fs/erofs/xattr.h b/fs/erofs/xattr.h
index 0a43c9ee9f8f..a94932c4938c 100644
--- a/fs/erofs/xattr.h
+++ b/fs/erofs/xattr.h
@@ -41,7 +41,7 @@ extern const struct xattr_handler erofs_xattr_user_handler;
extern const struct xattr_handler erofs_xattr_trusted_handler;
extern const struct xattr_handler erofs_xattr_security_handler;
-static inline const struct xattr_handler *erofs_xattr_handler(unsigned int idx)
+static inline const char *erofs_xattr_prefix(unsigned int idx, struct dentry *dentry)
{
static const struct xattr_handler *xattr_handler_map[] = {
[EROFS_XATTR_INDEX_USER] = &erofs_xattr_user_handler,
@@ -57,8 +57,11 @@ static inline const struct xattr_handler *erofs_xattr_handler(unsigned int idx)
#endif
};
- return idx && idx < ARRAY_SIZE(xattr_handler_map) ?
- xattr_handler_map[idx] : NULL;
+ if (idx && idx < ARRAY_SIZE(xattr_handler_map) &&
+ xattr_dentry_list(xattr_handler_map[idx], dentry))
+ return xattr_prefix(handler);
+
+ return NULL;
}
