On Thu, Aug 12, 2021 at 09:03:34AM +0100, Christoph Hellwig wrote: > On Tue, Aug 10, 2021 at 03:24:37PM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@xxxxxxxxxx> > > > > 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... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
