Re: [PATCH v11 01/25] xfs: Add xfs_has_attr and subroutines

[Dave Chinner <david@fromorbit.com>]

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@oracle.com>
> > Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> 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@fromorbit.com