On 7/23/20 7:24 PM, Dave Chinner wrote:
On Tue, Jul 21, 2020 at 04:26:13PM -0700, Darrick J. Wong wrote:
On Mon, Jul 20, 2020 at 05:15:42PM -0700, Allison Collins wrote:
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>
Reviewed-by: Chandan Rajendra <chandanrlinux@xxxxxxxxx>
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
Looks good enough for now... I still dislike generating ENOATTR/EEXIST
deep in the folds of the attr code but that's probably a bigger thing to
be wrangled with later. (And tbh I've thought about this & haven't come
up with a better idea anyway :P)
Yes, I agree it is hard to read, but I do think there's a cleaner
way of doing this. Take, for example, xfs_attr_leaf_try_add(). It
looks like this:
/*
* 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_attr_leaf_hasname(args, &bp);
if (retval != -ENOATTR && retval != -EEXIST)
return retval;
if (retval == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
goto out_brelse;
if (retval == -EEXIST) {
if (args->attr_flags & XATTR_CREATE)
goto out_brelse;
trace_xfs_attr_leaf_replace(args);
/* save the attribute state for later removal*/
args->op_flags |= XFS_DA_OP_RENAME; /* an atomic rename */
xfs_attr_save_rmt_blk(args);
/*
* clear the remote attr state now that it is saved so that the
* values reflect the state of the attribute we are about to
* add, not the attribute we just found and will remove later.
*/
args->rmtblkno = 0;
args->rmtblkcnt = 0;
args->rmtvaluelen = 0;
}
/*
* Add the attribute to the leaf block
*/
return xfs_attr3_leaf_add(bp, args);
out_brelse:
xfs_trans_brelse(args->trans, bp);
return retval;
}
I agree, the error handling is messy and really hard to follow.
But if we write it like this:
/*
* 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_attr_leaf_hasname(args, &bp);
switch (retval) {
case -ENOATTR:
if (args->attr_flags & XATTR_REPLACE)
goto out_brelse;
break;
case -EEXIST:
if (args->attr_flags & XATTR_CREATE)
goto out_brelse;
trace_xfs_attr_leaf_replace(args);
/* save the attribute state for later removal*/
args->op_flags |= XFS_DA_OP_RENAME; /* an atomic rename */
xfs_attr_save_rmt_blk(args);
/*
* clear the remote attr state now that it is saved so that the
* values reflect the state of the attribute we are about to
* add, not the attribute we just found and will remove later.
*/
args->rmtblkno = 0;
args->rmtblkcnt = 0;
args->rmtvaluelen = 0;
break;
case 0:
break;
default:
return retval;
}
/*
* Add the attribute to the leaf block
*/
return xfs_attr3_leaf_add(bp, args);
out_brelse:
xfs_trans_brelse(args->trans, bp);
return retval;
}
The logic is *much* cleaner and it is not overly verbose, either.
This sort of change could be done at the end of the series, too,
rather than requiring a rebase of everything....
Sure, I know this one had quite a bit of ping pong before it landed
where it did. I am not opposed to later rearranging it as long as the
underlying mechanics are the same. :-)
Allison
Cheers,
Dave.
