[Top] [All Lists]

Re: [PATCH] Give a transaction to xfs_attr_set_int

To: Niv Sardi <xaiki@xxxxxxx>
Subject: Re: [PATCH] Give a transaction to xfs_attr_set_int
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 30 Jun 2008 08:08:59 +1000
Cc: xfs@xxxxxxxxxxx, Niv Sardi <xaiki@xxxxxxxxxx>
In-reply-to: <1214196150-5427-5-git-send-email-xaiki@xxxxxxx>
Mail-followup-to: Niv Sardi <xaiki@xxxxxxx>, xfs@xxxxxxxxxxx, Niv Sardi <xaiki@xxxxxxxxxx>
References: <1214196150-5427-1-git-send-email-xaiki@xxxxxxx> <1214196150-5427-2-git-send-email-xaiki@xxxxxxx> <1214196150-5427-3-git-send-email-xaiki@xxxxxxx> <1214196150-5427-4-git-send-email-xaiki@xxxxxxx> <1214196150-5427-5-git-send-email-xaiki@xxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.17+20080114 (2008-01-14)
On Mon, Jun 23, 2008 at 02:42:30PM +1000, Niv Sardi wrote:
> From: Niv Sardi <xaiki@xxxxxxxxxx>
> We introduce xfs_trans_attr_set_int that takes a transaction pointer
> as an argument (or creates one if NULL) and only finishes the
> transaction if it has created it. We use xfs_attr_rolltrans to do the
> tranS_dup dance.
> xfs_attr_set_int is changed to a wrapper that will only call
> xfs_trans_attr_set_int with a NULL transaction.

As a general comment to the entire patch set, I dislike the
namespace pollution caused by changing xfs_attr_...  to

The xfs_trans_... namespace is used for stuff inside xfs_trans*[ch],
not for attr code. I'd suggest making the "trans" a suffix rather
than a prefix, or something similar, so that these attr functions
are not easily confused with core transaction code....


> @@ -356,6 +381,8 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int 
> namelen,
>                       if (!error && (flags & ATTR_KERNOTIME) == 0) {
>                               xfs_ichgtime(dp, XFS_ICHGTIME_CHG);
>                       }
> +                     if (tpp)
> +                             tpp = &args.trans;

That's busted too. Can you please review all the places where you
return transactio pointers to the caller via a function parameterrr
for this bug as you've made in at least a couple of places.

> +     if (tpp)
> +             tpp = &args.trans;

Here as well.

>       return(error);
>  out:
> @@ -434,10 +467,25 @@ out:
>               xfs_trans_cancel(args.trans,
>       xfs_iunlock(dp, XFS_ILOCK_EXCL);
> +     if (tpp)
> +             tpp = &args.trans;

And again.


Dave Chinner

<Prev in Thread] Current Thread [Next in Thread>