[3/3] xfs: actually check xfs_btree_check_block return in xfs_btree_islastblock
diff mbox series

Message ID 157319670439.834585.6578359830660435523.stgit@magnolia
State Superseded
Headers show
Series
  • xfs: various coverity fixes
Related show

Commit Message

Darrick J. Wong Nov. 8, 2019, 7:05 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 |    7 ++++++-
 fs/xfs/libxfs/xfs_btree.c |   19 ++++++++++++-------
 fs/xfs/libxfs/xfs_btree.h |    7 ++++---
 3 files changed, 22 insertions(+), 11 deletions(-)

Comments

Christoph Hellwig Nov. 8, 2019, 7:14 a.m. UTC | #1
On Thu, Nov 07, 2019 at 11:05:04PM -0800, Darrick J. Wong wrote:
> 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.

Could we just kill xfs_btree_islastblock?  It has pretty trivial, and
only has a single caller which only uses on of the two branches in the
function anyway.
Darrick J. Wong Nov. 8, 2019, 7:29 a.m. UTC | #2
On Thu, Nov 07, 2019 at 11:14:41PM -0800, Christoph Hellwig wrote:
> On Thu, Nov 07, 2019 at 11:05:04PM -0800, Darrick J. Wong wrote:
> > 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.
> 
> Could we just kill xfs_btree_islastblock?  It has pretty trivial, and
> only has a single caller which only uses on of the two branches in the
> function anyway.

I'd rather leave it as a btree primitive, honestly.

That said, "Is this cursor pointing to the last block on $level?" only
makes sense if you've already performed a lookup (or seek) operation.
If you've done that, you've already checked the block, right?  So I
think we could just get rid of the _check_block call on the grounds that
we already did that as part of the lookup (or turn it into an ASSERT),
and then this becomes a short enough function to try to make it a four
line static inline predicate.

Same result, but slightly better encapsulation.

(Yeah yeah, it's C, we're all one big happy family of bits...)

--D
Christoph Hellwig Nov. 8, 2019, 7:33 a.m. UTC | #3
On Thu, Nov 07, 2019 at 11:29:51PM -0800, Darrick J. Wong wrote:
> That said, "Is this cursor pointing to the last block on $level?" only
> makes sense if you've already performed a lookup (or seek) operation.
> If you've done that, you've already checked the block, right?  So I
> think we could just get rid of the _check_block call on the grounds that
> we already did that as part of the lookup (or turn it into an ASSERT),
> and then this becomes a short enough function to try to make it a four
> line static inline predicate.
> 
> Same result, but slightly better encapsulation.
> 
> (Yeah yeah, it's C, we're all one big happy family of bits...)

Sounds ok.
Darrick J. Wong Nov. 8, 2019, 7:46 a.m. UTC | #4
On Thu, Nov 07, 2019 at 11:33:06PM -0800, Christoph Hellwig wrote:
> On Thu, Nov 07, 2019 at 11:29:51PM -0800, Darrick J. Wong wrote:
> > That said, "Is this cursor pointing to the last block on $level?" only
> > makes sense if you've already performed a lookup (or seek) operation.
> > If you've done that, you've already checked the block, right?  So I
> > think we could just get rid of the _check_block call on the grounds that
> > we already did that as part of the lookup (or turn it into an ASSERT),
> > and then this becomes a short enough function to try to make it a four
> > line static inline predicate.
> > 
> > Same result, but slightly better encapsulation.
> > 
> > (Yeah yeah, it's C, we're all one big happy family of bits...)
> 
> Sounds ok.

Ok, I'll push out a (lightly tested) patch.

--D

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 3a724d80783a..65d870189548 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -1560,6 +1560,7 @@  xfs_alloc_ag_vextent_near(
 	int			i;		/* result code, temporary */
 	xfs_agblock_t		bno;
 	xfs_extlen_t		len;
+	bool			is_last;
 
 	/* handle uninitialized agbno range so caller doesn't have to */
 	if (!args->min_agbno && !args->max_agbno)
@@ -1603,7 +1604,11 @@  xfs_alloc_ag_vextent_near(
 	 * block, then we just examine all the entries in that block
 	 * that are big enough, and pick the best one.
 	 */
-	if (xfs_btree_islastblock(acur.cnt, 0)) {
+	error = xfs_btree_islastblock(acur.cnt, 0, &is_last);
+	if (error)
+		goto out;
+
+	if (is_last) {
 		bool		allocated = false;
 
 		error = xfs_alloc_ag_vextent_lastblock(args, &acur, &bno, &len,
diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 8ad153bc4dea..37b4fa93085c 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 */
+	xfs_btree_cur_t		*cur,
+	int			level,
+	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..c63da9dd05cc 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 */
+	xfs_btree_cur_t		*cur,
+	int			level,
+	bool			*last);
 
 /*
  * Compute first and last byte offsets for the fields given.