| Message ID | 157319670439.834585.6578359830660435523.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: various coverity fixes | expand |
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.
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
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.
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
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.