xfs: more return cleanups
cover xfs: more return cleanups
Hi folks,
I noticed a bit of inconsistency when reviewing the shift extent return cleanups that coccinelle flagged. The first was that the out_error labels were basically meaningless - it jsut returns the error, and we have much neater ways of doing that. Also, the logic flow through xfs_bmse_shift_one() jumped around all over the place - it can be done lots neater without any gotos at all, and is easier to read as a result.
These patches apply on top of the for-next branch (the xfs-coccinelle-cleanups branch to be precise).
Cheers,
Dave.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/2 xfs: cleanup xfs_bmse_shift_one goto mess
From: Dave Chinner <[email protected]>
xfs_bmse_shift_one() jumps around determining whether to shift or merge, making the code flow difficult to follow. Clean it up and use direct error returns (including XFS_WANT_CORRUPTED_RETURN) to make the code flow better and be easier to read.
--- fs/xfs/libxfs/xfs_bmap.c | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-)
fs/xfs/libxfs/xfs_bmap.c +17 -26
@@ -5544,35 +5544,29 @@ xfs_bmse_shift_one( startoff = got.br_startoff - offset_shift_fsb;/* delalloc extents should be prevented by caller */ -XFS_WANT_CORRUPTED_GOTO (!isnullstartblock (got.br_startblock), - out_error); +XFS_WANT_CORRUPTED_RETURN (!isnullstartblock (got.br_startblock));/* -* If this is the first extent in the file, make sure there's enough -* room at the start of the file and jump right to the shift as there's -* no left extent to merge. +* Check for merge if we've got an extent to the left, otherwise make +* sure there's enough room at the start of the file for the shift. */ -if (*current_ext ==0 ) { -if (got.br_startoff < offset_shift_fsb) -return -EINVAL ; - goto shift_extent; - } +if (*current_ext) { +/* grab the left extent and check for a large enough hole */ + leftp =xfs_iext_get_ext (ifp, *current_ext -1 ); +xfs_bmbt_get_all (leftp, &left); -/* grab the left extent and check for a large enough hole */ - leftp =xfs_iext_get_ext (ifp, *current_ext -1 ); -xfs_bmbt_get_all (leftp, &left); +if (startoff < left.br_startoff + left.br_blockcount) +return -EINVAL ; -if (startoff < left.br_startoff + left.br_blockcount) +/* check whether to merge the extent or shift it down */ +if (xfs_bmse_can_merge (&left, &got, offset_shift_fsb)) { +return xfs_bmse_merge (ip, whichfork, offset_shift_fsb, + *current_ext, gotp, leftp, cur, + logflags); + } + }else if (got.br_startoff < offset_shift_fsb)return -EINVAL ; -/* check whether to merge the extent or shift it down */ -if (!xfs_bmse_can_merge (&left, &got, offset_shift_fsb)) - goto shift_extent; - -return xfs_bmse_merge (ip, whichfork, offset_shift_fsb, *current_ext, - gotp, leftp, cur, logflags); - -shift_extent:/* * Increment the extent index for the next iteration, update the start * offset of the in-core extent and update the btree if applicable.
@@ -5589,14 +5583,11 @@ shift_extent: got.br_blockcount, &i);if (error)return error; -XFS_WANT_CORRUPTED_GOTO (i ==1 , out_error); +XFS_WANT_CORRUPTED_RETURN (i ==1 ); got.br_startoff = startoff;return xfs_bmbt_update (cur, got.br_startoff, got.br_startblock, got.br_blockcount, got.br_state); - -out_error: -return error; }/*
Signature · 6 lines
2.0.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 1/2] xfs: cleanup xfs_bmse_shift_one goto mess
Quoted 9 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_bmse_shift_one()jumps around determining whether to shift or merge, making the code flow difficult to follow. Clean it up and use direct error returns (includingXFS_WANT_CORRUPTED_RETURN) to make the code flow better and be easier to read.Signed-off-by Dave Chinner <[email protected]>---
Quoted 82 lines
fs/xfs/libxfs/xfs_bmap.c | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-)fs/xfs/libxfs/xfs_bmap.c +17
-26@@ -5544,35 +5544,29 @@ xfs_bmse_shift_one( startoff = got.br_startoff - offset_shift_fsb;/* delalloc extents should be prevented by caller */ -XFS_WANT_CORRUPTED_GOTO (!isnullstartblock (got.br_startblock), - out_error); +XFS_WANT_CORRUPTED_RETURN (!isnullstartblock (got.br_startblock));/* -* If this is the first extent in the file, make sure there's enough -* room at the start of the file and jump right to the shift as there's -* no left extent to merge. +* Check for merge if we've got an extent to the left, otherwise make +* sure there's enough room at the start of the file for the shift. */ -if (*current_ext ==0 ) { -if (got.br_startoff < offset_shift_fsb) -return -EINVAL ; - goto shift_extent; - } +if (*current_ext) { +/* grab the left extent and check for a large enough hole */ + leftp =xfs_iext_get_ext (ifp, *current_ext -1 ); +xfs_bmbt_get_all (leftp, &left); -/* grab the left extent and check for a large enough hole */ - leftp =xfs_iext_get_ext (ifp, *current_ext -1 ); -xfs_bmbt_get_all (leftp, &left); +if (startoff < left.br_startoff + left.br_blockcount) +return -EINVAL ; -if (startoff < left.br_startoff + left.br_blockcount) +/* check whether to merge the extent or shift it down */ +if (xfs_bmse_can_merge (&left, &got, offset_shift_fsb)) { +return xfs_bmse_merge (ip, whichfork, offset_shift_fsb, + *current_ext, gotp, leftp, cur, + logflags); + } + }else if (got.br_startoff < offset_shift_fsb)return -EINVAL ; -/* check whether to merge the extent or shift it down */ -if (!xfs_bmse_can_merge (&left, &got, offset_shift_fsb)) - goto shift_extent; - -return xfs_bmse_merge (ip, whichfork, offset_shift_fsb, *current_ext, - gotp, leftp, cur, logflags); - -shift_extent:/* * Increment the extent index for the next iteration, update the start * offset of the in-core extent and update the btree if applicable. @@ -5589,14 +5583,11 @@ shift_extent: got.br_blockcount, &i);if (error)return error; -XFS_WANT_CORRUPTED_GOTO (i ==1 , out_error); +XFS_WANT_CORRUPTED_RETURN (i ==1 ); got.br_startoff = startoff;return xfs_bmbt_update (cur, got.br_startoff, got.br_startblock, got.br_blockcount, got.br_state); - -out_error: -return error; }/* Signature · 6 lines
2.0.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/2 xfs: cleanup xfs_bmse_merge returns
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
xfs_bmse_merge() has a jump label for return that just returns the error value. Convert all the code to just return the error directly and use XFS_WANT_CORRUPTED_RETURN. This also allows the final call to xfs_bmbt_update() to return directly.
Noticed while reviewing coccinelle return cleanup patches and wondering why the same return pattern as in xfs_bmse_shift_one() wasn't picked up by the checker pattern...
--- fs/xfs/libxfs/xfs_bmap.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)
fs/xfs/libxfs/xfs_bmap.c +8 -15
@@ -5489,32 +5489,25 @@ xfs_bmse_merge( error =xfs_bmbt_lookup_eq (cur, got.br_startoff, got.br_startblock, got.br_blockcount, &i);if (error) - goto out_error; -XFS_WANT_CORRUPTED_GOTO (i ==1 , out_error); +return error; +XFS_WANT_CORRUPTED_RETURN (i ==1 ); error =xfs_btree_delete (cur, &i);if (error) - goto out_error; -XFS_WANT_CORRUPTED_GOTO (i ==1 , out_error); +return error; +XFS_WANT_CORRUPTED_RETURN (i ==1 );/* lookup and update size of the previous extent */ error =xfs_bmbt_lookup_eq (cur, left.br_startoff, left.br_startblock, left.br_blockcount, &i);if (error) - goto out_error; -XFS_WANT_CORRUPTED_GOTO (i ==1 , out_error); +return error; +XFS_WANT_CORRUPTED_RETURN (i ==1 ); left.br_blockcount = blockcount; - error =xfs_bmbt_update (cur, left.br_startoff, left.br_startblock, - left.br_blockcount, left.br_state); -if (error) - goto out_error; - -return 0 ; - -out_error: -return error; +return xfs_bmbt_update (cur, left.br_startoff, left.br_startblock, + left.br_blockcount, left.br_state); }/*
Signature · 6 lines
2.0.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/2] xfs: cleanup xfs_bmse_merge returns
Quoted 9 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Signed-off-by Dave Chinner <[email protected]>
xfs_bmse_merge()has a jump label for return that just returns the error value. Convert all the code to just return the error directly and useXFS_WANT_CORRUPTED_RETURN. This also allows the final call toxfs_bmbt_update()to return directly.
Quoted 6 lines
Noticed while reviewing coccinelle return cleanup patches and wondering why the same return pattern as in
xfs_bmse_shift_one()wasn't picked up by the checker pattern...Signed-off-by Dave Chinner <[email protected]>---
Quoted 55 lines
fs/xfs/libxfs/xfs_bmap.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)fs/xfs/libxfs/xfs_bmap.c +8
-15@@ -5489,32 +5489,25 @@ xfs_bmse_merge( error =xfs_bmbt_lookup_eq (cur, got.br_startoff, got.br_startblock, got.br_blockcount, &i);if (error) - goto out_error; -XFS_WANT_CORRUPTED_GOTO (i ==1 , out_error); +return error; +XFS_WANT_CORRUPTED_RETURN (i ==1 ); error =xfs_btree_delete (cur, &i);if (error) - goto out_error; -XFS_WANT_CORRUPTED_GOTO (i ==1 , out_error); +return error; +XFS_WANT_CORRUPTED_RETURN (i ==1 );/* lookup and update size of the previous extent */ error =xfs_bmbt_lookup_eq (cur, left.br_startoff, left.br_startblock, left.br_blockcount, &i);if (error) - goto out_error; -XFS_WANT_CORRUPTED_GOTO (i ==1 , out_error); +return error; +XFS_WANT_CORRUPTED_RETURN (i ==1 ); left.br_blockcount = blockcount; - error =xfs_bmbt_update (cur, left.br_startoff, left.br_startblock, - left.br_blockcount, left.br_state); -if (error) - goto out_error; - -return 0 ; - -out_error: -return error; +return xfs_bmbt_update (cur, left.br_startoff, left.br_startblock, + left.br_blockcount, left.br_state); }/* Signature · 6 lines
2.0.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs