Re: [PATCH v13 02/10] xfs: Add delay ready attr remove routines

Allison Henderson
2020-11-13T04:00:33Z

On 11/10/20 5:28 PM, Dave Chinner wrote:
> On Tue, Nov 10, 2020 at 03:43:31PM -0800, Darrick J. Wong wrote:
>> On Thu, Oct 22, 2020 at 11:34:27PM -0700, Allison Henderson wrote:
>>> +/*
>>> + * Remove the attribute specified in @args.
>>> + *
>>> + * This function may return -EAGAIN to signal that the transaction needs to be
>>> + * rolled. Callers should continue calling this function until they receive a
>>> + * return value other than -EAGAIN.
>>> + */
>>> +int
>>> +xfs_attr_remove_iter(
>>> + struct xfs_delattr_context *dac)
>>> +{
>>> + struct xfs_da_args *args = dac->da_args;
>>> + struct xfs_inode *dp = args->dp;
>>> +
>>> + if (dac->dela_state == XFS_DAS_RM_SHRINK)
>>> + goto node;
>>>
>>
>> Might as well just make this part of the if statement dispatch:
>>
>> if (dac->dela_state == XFS_DAS_RM_SHRINK)
>> return xfs_attr_node_removename_iter(dac);
>> else if (!xfs_inode_hasattr(dp))
>> return -ENOATTR;
>>
>>> if (!xfs_inode_hasattr(dp)) {
>>> - error = -ENOATTR;
>>> + return -ENOATTR;
>>> } else if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
>>> ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
>>> - error = xfs_attr_shortform_remove(args);
>>> + return xfs_attr_shortform_remove(args);
>>> } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>>> - error = xfs_attr_leaf_removename(args);
>>> - } else {
>>> - error = xfs_attr_node_removename(args);
>>> + return xfs_attr_leaf_removename(args);
>>> }
>>> -
>>> - return error;
>>> +node:
>>> + return xfs_attr_node_removename_iter(dac);
>
> Just a nitpick on this anti-pattern: else is not necessary
> when the branch returns.
>
> if (!xfs_inode_hasattr(dp))
> return -ENOATTR;
>
> if (dac->dela_state == XFS_DAS_RM_SHRINK)
> return xfs_attr_node_removename_iter(dac);
>
> if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
> ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
> return xfs_attr_shortform_remove(args);
> }
>
> if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> return xfs_attr_leaf_removename(args);
>
> return xfs_attr_node_removename_iter(dac);
>
> -Dave.
>
Sure, I think its ok to clean out the elses sense they all return. Thanks!

Allison