xfs
[Top] [All Lists]

Re: [PATCH] Introduce xfs_trans_bmap_add_attrfork.

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] Introduce xfs_trans_bmap_add_attrfork.
From: Niv Sardi <xaiki@xxxxxxx>
Date: Fri, 27 Jun 2008 14:42:17 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20080626092856.GA27069@xxxxxxxxxxxxx> (Christoph Hellwig's message of "Thu, 26 Jun 2008 05:28:56 -0400")
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> <20080626092856.GA27069@xxxxxxxxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Gnus/5.110011 (No Gnus v0.11) Emacs/23.0.60 (i486-pc-linux-gnu)
Updated patch attached,
Moved case where there is no transaction back into
xfs_bmap_add_attrfork() and rely on caller to call xfs_trans_roll(),

Christoph Hellwig <hch@xxxxxxxxxxxxx> writes:
>> +xfs_bmap_add_attrfork(
[...]
> Care to add this below xfs_trans_bmap_add_attrfork?  Also please
> us struct xfs_inode instead of the typedef.

Done,

>> +    if (tpp) {
>> +            ASSERT(*tpp);
>> +            tp = *tpp;
>> +    } else {
[...]
>
> I think it would be much cleaner if all the transaction setup, joining
> etc was done in xfs_bmap_add_attrfork, and xfs_trans_bmap_add_attrfork
> just gets an always non-NULL xfs_trans pointer.  That would mean
> the common code would become just a helper, and
> xfs_trans_bmap_add_attrfork would be a small wrapper including the 
> xfs_trans_roll.  Alternatively the caller could always do the roll.

I think I got it right, but error handeling is a bit weird this way,
looks logically identical.





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