* [PATCH 0/2] xfs: more return cleanups @ 2014-12-01 4:20 Dave Chinner 2014-12-01 4:20 ` [PATCH 1/2] xfs: cleanup xfs_bmse_shift_one goto mess Dave Chinner 2014-12-01 4:20 ` [PATCH 2/2] xfs: cleanup xfs_bmse_merge returns Dave Chinner 0 siblings, 2 replies; 5+ messages in thread From: Dave Chinner @ 2014-12-01 4:20 UTC (permalink / raw) To: xfs 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 xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] xfs: cleanup xfs_bmse_shift_one goto mess 2014-12-01 4:20 [PATCH 0/2] xfs: more return cleanups Dave Chinner @ 2014-12-01 4:20 ` Dave Chinner 2014-12-01 16:41 ` Brian Foster 2014-12-01 4:20 ` [PATCH 2/2] xfs: cleanup xfs_bmse_merge returns Dave Chinner 1 sibling, 1 reply; 5+ messages in thread From: Dave Chinner @ 2014-12-01 4:20 UTC (permalink / raw) To: xfs From: Dave Chinner <dchinner@redhat.com> 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. Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/libxfs/xfs_bmap.c | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 20d2e96..0628a67 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -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; } /* -- 2.0.0 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] xfs: cleanup xfs_bmse_shift_one goto mess 2014-12-01 4:20 ` [PATCH 1/2] xfs: cleanup xfs_bmse_shift_one goto mess Dave Chinner @ 2014-12-01 16:41 ` Brian Foster 0 siblings, 0 replies; 5+ messages in thread From: Brian Foster @ 2014-12-01 16:41 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs On Mon, Dec 01, 2014 at 03:20:07PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > 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. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/libxfs/xfs_bmap.c | 43 +++++++++++++++++-------------------------- > 1 file changed, 17 insertions(+), 26 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 20d2e96..0628a67 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -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; > } > > /* > -- > 2.0.0 > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] xfs: cleanup xfs_bmse_merge returns 2014-12-01 4:20 [PATCH 0/2] xfs: more return cleanups Dave Chinner 2014-12-01 4:20 ` [PATCH 1/2] xfs: cleanup xfs_bmse_shift_one goto mess Dave Chinner @ 2014-12-01 4:20 ` Dave Chinner 2014-12-01 16:41 ` Brian Foster 1 sibling, 1 reply; 5+ messages in thread From: Dave Chinner @ 2014-12-01 4:20 UTC (permalink / raw) To: xfs From: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <dchinner@redhat.com> 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... Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/libxfs/xfs_bmap.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 0628a67..5a42e2b 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -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); } /* -- 2.0.0 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] xfs: cleanup xfs_bmse_merge returns 2014-12-01 4:20 ` [PATCH 2/2] xfs: cleanup xfs_bmse_merge returns Dave Chinner @ 2014-12-01 16:41 ` Brian Foster 0 siblings, 0 replies; 5+ messages in thread From: Brian Foster @ 2014-12-01 16:41 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs On Mon, Dec 01, 2014 at 03:20:08PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > 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... > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/libxfs/xfs_bmap.c | 23 ++++++++--------------- > 1 file changed, 8 insertions(+), 15 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 0628a67..5a42e2b 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -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); > } > > /* > -- > 2.0.0 > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-12-01 16:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-01 4:20 [PATCH 0/2] xfs: more return cleanups Dave Chinner 2014-12-01 4:20 ` [PATCH 1/2] xfs: cleanup xfs_bmse_shift_one goto mess Dave Chinner 2014-12-01 16:41 ` Brian Foster 2014-12-01 4:20 ` [PATCH 2/2] xfs: cleanup xfs_bmse_merge returns Dave Chinner 2014-12-01 16:41 ` Brian Foster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox