diff mbox series

[3/6] xfs: actually check xfs_btree_check_block return in xfs_btree_islastblock

Message ID 157309576284.46520.16933998796526579184.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: various coverity fixes | expand

Commit Message

Darrick J. Wong Nov. 7, 2019, 3:02 a.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Coverity points out that xfs_btree_islastblock calls
xfs_btree_check_block, but doesn't act on an error return.  This
predicate has no answer if the btree is corrupt, so tweak the helper to
be able to return errors, and then modify the one call site.

Coverity-id: 114069
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc.c |    6 +++++-
 fs/xfs/libxfs/xfs_btree.c |   17 +++++++++++------
 fs/xfs/libxfs/xfs_btree.h |    5 +++--
 3 files changed, 19 insertions(+), 9 deletions(-)

Comments

Christoph Hellwig Nov. 7, 2019, 8:34 a.m. UTC | #1 
> -	while (xfs_btree_islastblock(acur.cnt, 0)) {
> +	error = xfs_btree_islastblock(acur.cnt, 0, &is_last);
> +	if (error)
> +		goto out;
> +	while (is_last) {

This transformation looks actually ok, but is highly non-obvious.
I think you want a prep patch just killing the pointless while first.
Darrick J. Wong Nov. 7, 2019, 8:41 p.m. UTC | #2 
On Thu, Nov 07, 2019 at 12:34:50AM -0800, Christoph Hellwig wrote:
> > -	while (xfs_btree_islastblock(acur.cnt, 0)) {
> > +	error = xfs_btree_islastblock(acur.cnt, 0, &is_last);
> > +	if (error)
> > +		goto out;
> > +	while (is_last) {
> 
> This transformation looks actually ok, but is highly non-obvious.
> I think you want a prep patch just killing the pointless while first.

Yeah, killing the while was a little more involved than I thought it
would be, but it's done.

--D
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index f7a4b54c5bc2..f609e1ab14d4 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -1479,6 +1479,7 @@  xfs_alloc_ag_vextent_near(
 	int			i;		/* result code, temporary */
 	xfs_agblock_t		bno;
 	xfs_extlen_t		len;
+	bool			is_last;
 #ifdef DEBUG
 	/*
 	 * Randomly don't execute the first algorithm.
@@ -1532,7 +1533,10 @@  xfs_alloc_ag_vextent_near(
 	 * This is written as a while loop so we can break out of it,
 	 * but we never loop back to the top.
 	 */
-	while (xfs_btree_islastblock(acur.cnt, 0)) {
+	error = xfs_btree_islastblock(acur.cnt, 0, &is_last);
+	if (error)
+		goto out;
+	while (is_last) {
 #ifdef DEBUG
 		if (dofirst)
 			break;
diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 98843f1258b8..098abfb50252 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -719,20 +719,25 @@  xfs_btree_get_bufs(
 /*
  * Check for the cursor referring to the last block at the given level.
  */
-int					/* 1=is last block, 0=not last block */
+int
 xfs_btree_islastblock(
 	xfs_btree_cur_t		*cur,	/* btree cursor */
-	int			level)	/* level to check */
+	int			level,	/* level to check */
+	bool			*last)
 {
 	struct xfs_btree_block	*block;	/* generic btree block pointer */
-	xfs_buf_t		*bp;	/* buffer containing block */
+	struct xfs_buf		*bp;	/* buffer containing block */
+	int			error;
 
 	block = xfs_btree_get_block(cur, level, &bp);
-	xfs_btree_check_block(cur, block, level, bp);
+	error = xfs_btree_check_block(cur, block, level, bp);
+	if (error)
+		return error;
 	if (cur->bc_flags & XFS_BTREE_LONG_PTRS)
-		return block->bb_u.l.bb_rightsib == cpu_to_be64(NULLFSBLOCK);
+		*last = block->bb_u.l.bb_rightsib == cpu_to_be64(NULLFSBLOCK);
 	else
-		return block->bb_u.s.bb_rightsib == cpu_to_be32(NULLAGBLOCK);
+		*last = block->bb_u.s.bb_rightsib == cpu_to_be32(NULLAGBLOCK);
+	return 0;
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
index 6670120cd690..ff54e6c18c44 100644
--- a/fs/xfs/libxfs/xfs_btree.h
+++ b/fs/xfs/libxfs/xfs_btree.h
@@ -320,10 +320,11 @@  xfs_btree_get_bufs(
 /*
  * Check for the cursor referring to the last block at the given level.
  */
-int					/* 1=is last block, 0=not last block */
+int
 xfs_btree_islastblock(
 	xfs_btree_cur_t		*cur,	/* btree cursor */
-	int			level);	/* level to check */
+	int			level,	/* level to check */
+	bool			*last);
 
 /*
  * Compute first and last byte offsets for the fields given.