On 2/23/20 5:20 AM, Amir Goldstein wrote:
On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
<allison.henderson@xxxxxxxxxx> wrote:
From: Allison Henderson <allison.henderson@xxxxxxxxxx>
This patch adds a new functions to check for the existence of an attribute.
Subroutines are also added to handle the cases of leaf blocks, nodes or shortform.
Common code that appears in existing attr add and remove functions have been
factored out to help reduce the appearance of duplicated code. We will need these
routines later for delayed attributes since delayed operations cannot return error
codes.
Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
---
fs/xfs/libxfs/xfs_attr.c | 171 ++++++++++++++++++++++++++++--------------
fs/xfs/libxfs/xfs_attr.h | 1 +
fs/xfs/libxfs/xfs_attr_leaf.c | 111 +++++++++++++++++----------
fs/xfs/libxfs/xfs_attr_leaf.h | 3 +
4 files changed, 188 insertions(+), 98 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 9acdb23..2255060 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -46,6 +46,7 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args);
STATIC int xfs_attr_leaf_get(xfs_da_args_t *args);
STATIC int xfs_attr_leaf_addname(xfs_da_args_t *args);
STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args);
+STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
/*
* Internal routines when attribute list is more than one block.
@@ -53,6 +54,8 @@ STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args);
STATIC int xfs_attr_node_get(xfs_da_args_t *args);
STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
+STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
+ struct xfs_da_state **state);
STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
STATIC int xfs_attr_refillstate(xfs_da_state_t *state);
@@ -310,6 +313,37 @@ xfs_attr_set_args(
}
/*
+ * Return EEXIST if attr is found, or ENOATTR if not
This is a very silly return value for a function named has_attr in my taste.
I realize you inherited this interface from xfs_attr3_leaf_lookup_int(), but
IMO this change looks like a very good opportunity to change that internal
API:
xfs_has_attr?
0: NO
1: YES (or stay with the syscall standard of -ENOATTR)
<0: error
Darrick had mentioned something like that in the last revision, but I
think people wanted to keep everything a straight hoist at this phase.
At least as much as possible. Maybe I can add another patch that does
this at the end when we're doing clean ups.
Allison
+ */
+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_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
+ ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
+ return xfs_attr_sf_findname(args, NULL, NULL);
+ }
+
+ if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
+ 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.
*/
int
@@ -583,26 +617,20 @@ STATIC int
xfs_attr_leaf_addname(
struct xfs_da_args *args)
{
- struct xfs_inode *dp;
struct xfs_buf *bp;
int retval, error, forkoff;
+ struct xfs_inode *dp = args->dp;
trace_xfs_attr_leaf_addname(args);
/*
- * Read the (only) block in the attribute list in.
- */
- dp = args->dp;
- args->blkno = 0;
- error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno, &bp);
- if (error)
- return error;
-
- /*
* Look up the given attribute in the leaf block. Figure out if
* the given flags produce an error or call for an atomic rename.
*/
- retval = xfs_attr3_leaf_lookup_int(bp, args);
+ retval = xfs_attr_leaf_hasname(args, &bp);
+ if (retval != -ENOATTR && retval != -EEXIST)
+ return retval;
+
if ((args->name.type & ATTR_REPLACE) && (retval == -ENOATTR)) {
Example of how sane code (in my taste) would look like:
retval = xfs_attr_leaf_hasname(args, &bp);
if (retval < 0)
return retval;
if ((args->name.type & ATTR_REPLACE) && !retval) {
xfs_trans_brelse(args->trans, bp);
return -ENOATTR;
} else if (retval) {
if (args->flags & ATTR_CREATE) { /* pure create op */
xfs_trans_brelse(args->trans, bp);
return -EEXIST;
}
Thanks,
Amir.
