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.
