Re: [PATCH 02/16] xfs: rename xfs_has_attr()

[Christoph Hellwig <hch@xxxxxxxxxxxxx>]

On Wed, Jul 14, 2021 at 02:18:58PM +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.

I agree with all the problems, and add to it that the calling
conventions are a little stupid, and the new name isn't all the great
either. I'd suggest to just remove it entirely:

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index d9d7d5137b73f6..181d3ad2239bc3 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -620,35 +620,6 @@ xfs_attr_set_iter(
 }
 
 
-/*
- * Return EEXIST if attr is found, or ENOATTR if not
- */
-int
-xfs_has_attr(
-	struct xfs_da_args	*args)
-{
-	struct xfs_inode	*dp = args->dp;
-	struct xfs_buf		*bp = NULL;
-	int			error;
-
-	if (!xfs_inode_hasattr(dp))
-		return -ENOATTR;
-
-	if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL)
-		return xfs_attr_sf_findname(args, NULL, NULL);
-
-	if (xfs_attr_is_leaf(dp)) {
-		error = xfs_attr_leaf_hasname(args, &bp);
-
-		if (bp)
-			xfs_trans_brelse(args->trans, bp);
-
-		return error;
-	}
-
-	return xfs_attr_node_hasname(args, NULL);
-}
-
 /*
  * Remove the attribute specified in @args.
  */
@@ -761,8 +732,21 @@ xfs_attr_set(
 			goto out_trans_cancel;
 	}
 
+	if (!xfs_inode_hasattr(dp)) {
+		error = -ENOATTR;
+	} else if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
+		error = xfs_attr_sf_findname(args, NULL, NULL);
+	} else if (xfs_attr_is_leaf(dp)) {
+		struct xfs_buf		*bp = NULL;
+
+		error = xfs_attr_leaf_hasname(args, &bp);
+		if (bp)
+			xfs_trans_brelse(args->trans, bp);
+	} else {
+		error = xfs_attr_node_hasname(args, NULL);
+	}
+
 	if (args->value) {
-		error = xfs_has_attr(args);
 		if (error == -EEXIST && (args->attr_flags & XATTR_CREATE))
 			goto out_trans_cancel;
 		if (error == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
@@ -777,7 +761,6 @@ xfs_attr_set(
 		if (!args->trans)
 			goto out_unlock;
 	} else {
-		error = xfs_has_attr(args);
 		if (error != -EEXIST)
 			goto out_trans_cancel;
 
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 8de5d1d2733ead..5e71f719bdd52b 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -490,7 +490,6 @@ int xfs_attr_get_ilocked(struct xfs_da_args *args);
 int xfs_attr_get(struct xfs_da_args *args);
 int xfs_attr_set(struct xfs_da_args *args);
 int xfs_attr_set_args(struct xfs_da_args *args);
-int xfs_has_attr(struct xfs_da_args *args);
 int xfs_attr_remove_args(struct xfs_da_args *args);
 int xfs_attr_remove_iter(struct xfs_delattr_context *dac);
 bool xfs_attr_namecheck(const void *name, size_t length);