On Wed, Aug 18, 2021 at 10:56:08AM +1000, Dave Chinner wrote:

On Thu, Aug 12, 2021 at 09:03:34AM +0100, Christoph Hellwig wrote:

quoted

On Tue, Aug 10, 2021 at 03:24:37PM +1000, Dave Chinner wrote:

quoted

From: Dave Chinner <redacted>

xfs_has_attr() is poorly named. It has global scope as it is defined
in a header file, but it has no namespace scope that tells us what
it is checking has attributes. It's not even clear what "has_attr"
means, because what it is actually doing is an attribute fork lookup
to see if the attribute exists.

Upcoming patches use this "xfs_has_<foo>" namespace for global
filesystem features, which conflicts with this function.

Rename xfs_has_attr() to xfs_attr_lookup() and make it a static
function, freeing up the "xfs_has_" namespace for global scope
usage.

Why not kill this function entirely as I suggested last time?

Because I think it's the wrong cleanup to apply to xfs_attr_set().
xfs_attr_set() needs to be split into two - a set() function and a
remove() function to get rid of all the conditional "if
(arg->value)" logic in it that separates set from remove. Most
of the code in the function is under such if/else clauses, and the
set() code is much more complex than the remove() case. Folding the
attr lookup into the xfs_attr_set() doesn't do anything to address
this high level badness, and to split it appropriately we need to
keep the common attr lookup code in it's own function.

I updated the patch to has a single xfs_attr_lookup() call instead
of one per branch, but I don't think removing the helper is the
right way to go here...

I prefer we leave the xattr code alone (renaming things to avoid name
conflicts is ok) and concentrated on helping Allison land xattr logging.
Once that's done, xfs_attr_set and xfs_attr_remove become trivial
frontends to a deferred operation.

--D

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com