On Wed, Sep 26, 2018 at 03:14:41AM -0700, Allison Henderson wrote:
> This patch adds a subroutine xfs_attr_try_sf_addname
> used by xfs_attr_set. This subrotine will attempt to
> add the attribute name specified in args in shortform,
> as well and perform error handling previously done in
> xfs_attr_set.
>
> This patch helps to pre-simplify xfs_attr_set for reviewing
> purposes and reduce indentation. New function will be added
> in the next patch.
>
> Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_attr.c | 61 +++++++++++++++++++++++++++++++-----------------
> 1 file changed, 40 insertions(+), 21 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 1e671d4..f0675be 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -191,6 +191,42 @@ xfs_attr_calc_size(
> return nblks;
> }
>
> +STATIC int
> +xfs_attr_try_sf_addname(
> + struct xfs_inode *dp,
> + struct xfs_da_args *args)
> +{
> +
> + struct xfs_mount *mp = dp->i_mount;
> + int error;
> +
> + error = xfs_attr_shortform_addname(args);
I wonder, what error codes can come out of this function? -ENOSPC,
-EEXIST, zero...?
> + if (error == -ENOSPC)
> + return error;
> +
> + /*
> + * Commit the shortform mods, and we're done.
> + * NOTE: this is also the error path
> + * (EEXIST, etc).
> + */
> + ASSERT(args->trans != NULL);
<urk> Um... I get that this is just mechanical code refactoring, but...
> +
> + /*
> + * If this is a synchronous mount, make sure
> + * that the transaction goes to disk before
> + * returning to the user.
> + */
> + if (mp->m_flags & XFS_MOUNT_WSYNC)
> + xfs_trans_set_sync(args->trans);
...let's say we're on the error path. Why would we care about making the
transaction synchronous?
> +
> + if (!error && (args->flags & ATTR_KERNOTIME) == 0) {
> + xfs_trans_ichgtime(args->trans, dp,
> + XFS_ICHGTIME_CHG);
> + }
And what's the point of updating ctime if we do?
And can we restructure this to follow the usual pattern:
error = xfs_blabhablhablah(...);
if (error)
bail out;
<keep going>
> +
> + return error;
> +}
> +
> int
> xfs_attr_set(
> struct xfs_inode *dp,
> @@ -281,30 +317,13 @@ xfs_attr_set(
> * Try to add the attr to the attribute list in
> * the inode.
> */
> - error = xfs_attr_shortform_addname(&args);
> + error = xfs_attr_try_sf_addname(dp, &args);
> if (error != -ENOSPC) {
> - /*
> - * Commit the shortform mods, and we're done.
> - * NOTE: this is also the error path (EEXIST, etc).
> - */
> - ASSERT(args.trans != NULL);
> -
> - /*
> - * If this is a synchronous mount, make sure that
> - * the transaction goes to disk before returning
> - * to the user.
> - */
> - if (mp->m_flags & XFS_MOUNT_WSYNC)
> - xfs_trans_set_sync(args.trans);
> -
> - if (!error && (flags & ATTR_KERNOTIME) == 0) {
> - xfs_trans_ichgtime(args.trans, dp,
> - XFS_ICHGTIME_CHG);
> - }
> err2 = xfs_trans_commit(args.trans);
> - xfs_iunlock(dp, XFS_ILOCK_EXCL);
> + error = error ? error : err2;
>
> - return error ? error : err2;
> + xfs_iunlock(dp, XFS_ILOCK_EXCL);
> + return error;
Huh, so we commit the transaction on error?? Why?
/me runs away
--D
> }
>
> /*
> --
> 2.7.4
>
