On Tue, Jan 05, 2016 at 01:01:48PM -0600, Eric Sandeen wrote:
> Calls to xfs_bmap_finish() and xfs_trans_ijoin(), and the
> associated comments were replicated several times across
> the attribute code, all dealing with what to do if the
> transaction was or wasn't committed.
>
> And in that replicated code, an ASSERT() test of an
> uninitialized variable occurs in several locations:
>
> error = xfs_attr_thing(&args);
> if (!error) {
> error = xfs_bmap_finish(&args.trans, args.flist,
> &committed);
> }
> if (error) {
> ASSERT(committed);
>
> If the first xfs_attr_thing() failed, we'd skip the xfs_bmap_finish,
> never set "committed", and then test it in the ASSERT.
>
> Fix this up by moving the committed state internal to xfs_bmap_finish,
> and add a new inode argument. If an inode is passed in, it is passed
> through to __xfs_trans_roll() and joined to the transaction there if
> the transaction was committed.
>
> xfs_qm_dqalloc() was a little unique in that it called bjoin rather
> than ijoin, but as Dave points out we can detect the committed state
> but checking whether (*tpp != tp).
>
> Addresses-Coverity-Id: 102360
> Addresses-Coverity-Id: 102361
> Addresses-Coverity-Id: 102363
> Addresses-Coverity-Id: 102364
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
>
> libxfs/xfs_attr.c | 114
> +++++------------------------------------------
> libxfs/xfs_attr_remote.c | 25 ----------
> libxfs/xfs_bmap.c | 6 --
> libxfs/xfs_bmap.h | 2
> xfs_bmap_util.c | 28 ++++-------
> xfs_dquot.c | 12 ++--
> xfs_inode.c | 22 ++-------
> xfs_iomap.c | 10 +---
> xfs_rtalloc.c | 3 -
> xfs_symlink.c | 12 ----
> 10 files changed, 50 insertions(+), 184 deletions(-)
Nice!
A few really minor things (repeated) to clean up the code being
touched a bit more.
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index f949818..e16aa32 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -207,7 +207,7 @@ xfs_attr_set(
> struct xfs_trans_res tres;
> xfs_fsblock_t firstblock;
> int rsvd = (flags & ATTR_ROOT) != 0;
> - int error, err2, committed, local;
> + int error, err2, local;
>
> XFS_STATS_INC(mp, xs_attr_set);
>
> @@ -335,24 +335,15 @@ xfs_attr_set(
> xfs_bmap_init(args.flist, args.firstblock);
> error = xfs_attr_shortform_to_leaf(&args);
> if (!error) {
> - error = xfs_bmap_finish(&args.trans, args.flist,
> - &committed);
> + error = xfs_bmap_finish(&args.trans, args.flist, dp);
> }
You can kill the {} on this if as well. (repeats)
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -91,32 +91,32 @@ xfs_zero_extent(
> * last due to locking considerations. We never free any extents in
> * the first transaction.
> *
> - * Return 1 if the given transaction was committed and a new one
> - * started, and 0 otherwise in the committed parameter.
> + * If an inode *ip is provided, rejoin it to the transaction if
> + * the transaction was committed.
> */
> int /* error */
> xfs_bmap_finish(
> struct xfs_trans **tp, /* transaction pointer addr */
> struct xfs_bmap_free *flist, /* i/o: list extents to free */
> - int *committed)/* xact committed or not */
> + xfs_inode_t *ip)
struct xfs_inode
> @@ -1071,7 +1067,7 @@ xfs_alloc_file_space(
> /*
> * Complete the transaction
> */
> - error = xfs_bmap_finish(&tp, &free_list, &committed);
> + error = xfs_bmap_finish(&tp, &free_list, NULL);
> if (error) {
> goto error0;
> }
Kill the {} here while touching the line above.
> @@ -1353,7 +1348,7 @@ xfs_free_file_space(
> /*
> * complete the transaction
> */
> - error = xfs_bmap_finish(&tp, &free_list, &committed);
> + error = xfs_bmap_finish(&tp, &free_list, NULL);
> if (error) {
> goto error0;
> }
And here.
> + int nmaps, error;
> xfs_buf_t *bp;
> xfs_trans_t *tp = *tpp;
>
> @@ -379,11 +379,11 @@ xfs_qm_dqalloc(
>
> xfs_trans_bhold(tp, bp);
>
> - if ((error = xfs_bmap_finish(tpp, &flist, &committed))) {
> + if ((error = xfs_bmap_finish(tpp, &flist, NULL)))
> goto error1;
> - }
error = xfs_bmap_finish(tpp, &flist, NULL);
if (error)
goto error1;
> @@ -1841,7 +1835,7 @@ xfs_inactive_ifree(
> * Just ignore errors at this point. There is nothing we can do except
> * to try to keep going. Make sure it's not a silent error.
> */
> - error = xfs_bmap_finish(&tp, &free_list, &committed);
> + error = xfs_bmap_finish(&tp, &free_list, NULL);
^^
You can fix the extra whitespace here, too.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
|