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....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
