| Message ID | 20230125-fs-acl-remove-generic-xattr-handlers-v1-5-6cf155b492b6@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acl: remove remaining posix acl handlers | expand |
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.
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; }
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.
diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c index a62fb8a3318a..a787e74d9a21 100644 --- a/fs/erofs/xattr.c +++ b/fs/erofs/xattr.c @@ -469,10 +469,6 @@ const struct xattr_handler __maybe_unused erofs_xattr_security_handler = { const struct xattr_handler *erofs_xattr_handlers[] = { &erofs_xattr_user_handler, -#ifdef CONFIG_EROFS_FS_POSIX_ACL - &posix_acl_access_xattr_handler, - &posix_acl_default_xattr_handler, -#endif &erofs_xattr_trusted_handler, #ifdef CONFIG_EROFS_FS_SECURITY &erofs_xattr_security_handler, @@ -480,6 +476,43 @@ const struct xattr_handler *erofs_xattr_handlers[] = { NULL, }; +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); + + return name; +} + struct listxattr_iter { struct xattr_iter it; @@ -496,13 +529,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..9376cbdc32d8 100644 --- a/fs/erofs/xattr.h +++ b/fs/erofs/xattr.h @@ -40,27 +40,6 @@ static inline unsigned int xattrblock_offset(struct erofs_sb_info *sbi, 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 const struct xattr_handler *xattr_handler_map[] = { - [EROFS_XATTR_INDEX_USER] = &erofs_xattr_user_handler, -#ifdef CONFIG_EROFS_FS_POSIX_ACL - [EROFS_XATTR_INDEX_POSIX_ACL_ACCESS] = - &posix_acl_access_xattr_handler, - [EROFS_XATTR_INDEX_POSIX_ACL_DEFAULT] = - &posix_acl_default_xattr_handler, -#endif - [EROFS_XATTR_INDEX_TRUSTED] = &erofs_xattr_trusted_handler, -#ifdef CONFIG_EROFS_FS_SECURITY - [EROFS_XATTR_INDEX_SECURITY] = &erofs_xattr_security_handler, -#endif - }; - - return idx && idx < ARRAY_SIZE(xattr_handler_map) ? - xattr_handler_map[idx] : NULL; -} - extern const struct xattr_handler *erofs_xattr_handlers[]; int erofs_getxattr(struct inode *, int, const char *, void *, size_t);
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: <linux-erofs@lists.ozlabs.org> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> --- fs/erofs/xattr.c | 49 +++++++++++++++++++++++++++++++++++++++---------- fs/erofs/xattr.h | 21 --------------------- 2 files changed, 39 insertions(+), 31 deletions(-)