misc: fix more stupid compiler warnings
PATCH 1/1 misc: fix more stupid compiler warnings
Fix more compiler warnings about pointless checks, unchecked return values, brace problems, and missing parentheses.
--- db/check.c | 2 -- libxfs/init.c | 17 ++++++++++++----- libxfs/libxfs_priv.h | 4 ++-- libxfs/xfs_bmap.c | 2 +- repair/phase6.c | 4 ++-- repair/rmap.c | 2 +- 6 files changed, 18 insertions(+), 13 deletions(-)
db/check.c +0 -2
@@ -3220,9 +3220,7 @@ process_leaf_node_dir_v2_free(return ; }if (be32_to_cpu (free->hdr.nvalid) > maxent || -be32_to_cpu (free->hdr.nvalid) <0 ||be32_to_cpu (free->hdr.nused) > maxent || -be32_to_cpu (free->hdr.nused) <0 ||be32_to_cpu (free->hdr.nused) >be32_to_cpu (free->hdr.nvalid)) {if (!sflag || v)
libxfs/init.c +12 -5
@@ -262,7 +262,10 @@ libxfs_init(libxfs_init_t *a) a->dsize = a->lbsize = a->rtbsize =0 ; a->dbsize = a->logBBsize = a->logBBstart = a->rtsize =0 ; - (void )getcwd (curdir,MAXPATHLEN ); +if (!getcwd (curdir,MAXPATHLEN )) { +perror ("getcwd" ); +strcpy (curdir,"." ); + } needcd =0 ; fd =-1 ; flags = (a->isreadonly | a->isdirect);
@@ -284,7 +287,8 @@ libxfs_init(libxfs_init_t *a) }if (dname) {if (dname[0 ] !='/' && needcd) -chdir (curdir); +if (chdir (curdir)) +perror (curdir);if (a->disfile) { a->ddev=libxfs_device_open (dname, a->dcreat, flags, a->setblksize);
@@ -305,7 +309,8 @@ libxfs_init(libxfs_init_t *a) a->dsize =0 ;if (logname) {if (logname[0 ] !='/' && needcd) -chdir (curdir); +if (chdir (curdir)) +perror (curdir);if (a->lisfile) { a->logdev =libxfs_device_open (logname, a->lcreat, flags, a->setblksize);
@@ -326,7 +331,8 @@ libxfs_init(libxfs_init_t *a) a->logBBsize =0 ;if (rtname) {if (rtname[0 ] !='/' && needcd) -chdir (curdir); +if (chdir (curdir)) +perror (curdir);if (a->risfile) { a->rtdev =libxfs_device_open (rtname, a->rcreat, flags, a->setblksize);
@@ -361,7 +367,8 @@ libxfs_init(libxfs_init_t *a) goto done; }if (needcd) -chdir (curdir); +if (chdir (curdir)) +perror (curdir);if (!libxfs_bhash_size) libxfs_bhash_size =LIBXFS_BHASHSIZE (sbp); libxfs_bcache = cache_init(a->bcache_flags, libxfs_bhash_size,
libxfs/libxfs_priv.h +2 -2
@@ -504,8 +504,8 @@ int libxfs_zero_extent(struct xfs_inode *ip, xfs_fsblock_t start_fsb,bool xfs_log_check_lsn (struct xfs_mount *,xfs_lsn_t );/* xfs_icache.c */ -#define xfs_inode_set_cowblocks_tag (ip) -#define xfs_inode_set_eofblocks_tag (ip) +#define xfs_inode_set_cowblocks_tag (ip) do { } while (0) +#define xfs_inode_set_eofblocks_tag (ip) do { } while (0)/* xfs_stats.h */ #define XFS_STATS_CALC_INDEX (member) 0
libxfs/xfs_bmap.c +1 -1
@@ -570,7 +570,7 @@ xfs_bmap_validate_ret(#else #define xfs_bmap_check_leaf_extents (cur, ip, whichfork) do { } while (0) -#define xfs_bmap_validate_ret (bno,len,flags,mval,onmap,nmap) +#define xfs_bmap_validate_ret (bno,len,flags,mval,onmap,nmap) do { } while (0)#endif /* DEBUG */ /*
repair/phase6.c +2 -2
@@ -3092,11 +3092,11 @@ mark_standalone_inodes(xfs_mount_t *mp) irec =find_inode_rec (mp,XFS_INO_TO_AGNO (mp, mp->m_sb.sb_rsumino),XFS_INO_TO_AGINO (mp, mp->m_sb.sb_rsumino)); +ASSERT (irec !=NULL ); + offset =XFS_INO_TO_AGINO (mp, mp->m_sb.sb_rsumino) - irec->ino_startnum; -ASSERT (irec !=NULL ); -add_inode_reached (irec, offset);if (fs_quotas) {
repair/rmap.c +1 -1
@@ -1542,7 +1542,7 @@ rmap_populate_realtime_rmapbt( imap.br_startoff = rm_rec->rm_offset; imap.br_startblock = rm_rec->rm_startblock; imap.br_blockcount = rm_rec->rm_blockcount; - imap.br_state = (rm_rec->rm_flags &XFS_RMAP_UNWRITTEN ? + imap.br_state = ((rm_rec->rm_flags &XFS_RMAP_UNWRITTEN ) ?XFS_EXT_UNWRITTEN :XFS_EXT_NORM ); fakei.i_ino = rm_rec->rm_owner; error = -libxfs_rmap_map_extent(mp, &dfops, &fakei,
Quoted 32 lines · Darrick J. Wong
Fix more compiler warnings about pointless checks, unchecked return values, brace problems, and missing parentheses.
Signed-off-by Darrick J. Wong <[email protected]>--- db/check.c | 2 -- libxfs/init.c | 17 ++++++++++++----- libxfs/libxfs_priv.h | 4 ++-- libxfs/xfs_bmap.c | 2 +- repair/phase6.c | 4 ++-- repair/rmap.c | 2 +- 6 files changed, 18 insertions(+), 13 deletions(-)db/check.c +0
-2@@ -3220,9 +3220,7 @@ process_leaf_node_dir_v2_free(return ; }if (be32_to_cpu (free->hdr.nvalid) > maxent || -be32_to_cpu (free->hdr.nvalid) <0 ||be32_to_cpu (free->hdr.nused) > maxent || -be32_to_cpu (free->hdr.nused) <0 ||be32_to_cpu (free->hdr.nused) >be32_to_cpu (free->hdr.nvalid)) {if (!sflag || v)libxfs/init.c +0
-0
libxfs/init.c libxfs_init(libxfs_init_t *a) · line 268
Quoted 8 lines
libxfs/init.c +4
-1@@ -262,7 +262,10 @@ libxfs_init(libxfs_init_t *a) a->dsize = a->lbsize = a->rtbsize =0 ; a->dbsize = a->logBBsize = a->logBBstart = a->rtsize =0 ; - (void )getcwd (curdir,MAXPATHLEN ); +if (!getcwd (curdir,MAXPATHLEN )) { +perror ("getcwd" ); +strcpy (curdir,"." ); + }
I can't quite figure out what this curdir/needcd stuff is for, but assuming it's not pointless, crazy cruft, I don't think this is safe; later code does "chdir(curdir)" and chdir(".") isn't going to do what the code expects, is it?
libxfs/init.c libxfs_init(libxfs_init_t *a) · line 291
Quoted 10 lines
libxfs/init.c +2
-1needcd =0 ; fd =-1 ; flags = (a->isreadonly | a->isdirect);@@ -284,7 +287,8 @@ libxfs_init(libxfs_init_t *a) }if (dname) {if (dname[0 ] !='/' && needcd) -chdir (curdir); +if (chdir (curdir)) +perror (curdir);
and again, if we fail, we print something and carry on?
It may shut up gcc, but this doesn't look like error handling to me :)
Could you tell what this chdir/curdir/needcd stuff is for?
-Eric
Can you splits this up a bit to be more self explanatory, e.g. one patch per warning class or logical change?
db/check.c process_leaf_node_dir_v2_free( · line 3225
Quoted 6 lines
db/check.c +0
-2if (be32_to_cpu (free->hdr.nvalid) > maxent || -be32_to_cpu (free->hdr.nvalid) <0 ||be32_to_cpu (free->hdr.nused) > maxent || -be32_to_cpu (free->hdr.nused) <0 ||be32_to_cpu (free->hdr.nused) >be32_to_cpu (free->hdr.nvalid)) {
be32_to_cpu returns uint, so this makese sense.
libxfs/init.c libxfs_init(libxfs_init_t *a) · line 268
Quoted 5 lines
libxfs/init.c +4
-1- (void )getcwd (curdir,MAXPATHLEN ); +if (!getcwd (curdir,MAXPATHLEN )) { +perror ("getcwd" ); +strcpy (curdir,"." ); + }
All this chdir magic will need a lot more explanation. To me it seems like most of this is a left over from IRIX days where we could have device names relative to a volume. The proper fix probably is to just remove that whole thing - if we'd ever need to bring it back we should use openat relative to a dirfd instead.
libxfs/libxfs_priv.h int libxfs_zero_extent(struct xfs_inode *ip, xfs_fsblock_t start_fsb, · line 508
Quoted 2 lines
libxfs/libxfs_priv.h +2
-0+#define xfs_inode_set_cowblocks_tag (ip) do { } while (0) +#define xfs_inode_set_eofblocks_tag (ip) do { } while (0)
This part looks fine.
libxfs/xfs_bmap.c xfs_bmap_validate_ret( · line 574
Quoted 5 lines
#else
#definexfs_bmap_check_leaf_extents(cur, ip, whichfork)do { } while (0)libxfs/xfs_bmap.c +1
-1-#define xfs_bmap_validate_ret (bno,len,flags,mval,onmap,nmap) +#define xfs_bmap_validate_ret (bno,len,flags,mval,onmap,nmap) do { } while (0)#endif /* DEBUG */
This as well.
Quoted 5 lines
repair/phase6.c +0
-0
repair/phase6.c mark_standalone_inodes(xfs_mount_t *mp) · line 3099
Quoted 10 lines
repair/phase6.c +2
-2@@ -3092,11 +3092,11 @@ mark_standalone_inodes(xfs_mount_t *mp) irec =find_inode_rec (mp,XFS_INO_TO_AGNO (mp, mp->m_sb.sb_rsumino),XFS_INO_TO_AGINO (mp, mp->m_sb.sb_rsumino)); +ASSERT (irec !=NULL ); + offset =XFS_INO_TO_AGINO (mp, mp->m_sb.sb_rsumino) - irec->ino_startnum; -ASSERT (irec !=NULL ); -
What's the point in even having this assert? Either we turn this
into something like
if (!irec) abort();
or just let the code dereference it.
repair/phase6.c mark_standalone_inodes(xfs_mount_t *mp) · line 3101
Quoted 3 lines
repair/phase6.c +1
-1- imap.br_state = (rm_rec->rm_flags &XFS_RMAP_UNWRITTEN ? + imap.br_state = ((rm_rec->rm_flags &XFS_RMAP_UNWRITTEN ) ?XFS_EXT_UNWRITTEN :XFS_EXT_NORM );
Looks fine.
Quoted 12 lines · Christoph Hellwig
Can you splits this up a bit to be more self explanatory, e.g. one patch per warning class or logical change?
db/check.c process_leaf_node_dir_v2_free( · line 3225
Quoted 6 lines
db/check.c +0
-2if (be32_to_cpu (free->hdr.nvalid) > maxent || -be32_to_cpu (free->hdr.nvalid) <0 ||be32_to_cpu (free->hdr.nused) > maxent || -be32_to_cpu (free->hdr.nused) <0 ||be32_to_cpu (free->hdr.nused) >be32_to_cpu (free->hdr.nvalid)) {
be32_to_cpureturns uint, so this makese sense.
Quoted 12 lines
libxfs/init.c libxfs_init(libxfs_init_t *a) · line 268
Quoted 5 lines
libxfs/init.c +4
-1- (void )getcwd (curdir,MAXPATHLEN ); +if (!getcwd (curdir,MAXPATHLEN )) { +perror ("getcwd" ); +strcpy (curdir,"." ); + }All this chdir magic will need a lot more explanation. To me it seems like most of this is a left over from IRIX days where we could have device names relative to a volume. The proper fix probably is to just remove that whole thing - if we'd ever need to bring it back we should use openat relative to a dirfd instead.
Yeah, AFAICT all this getcwd/chdir stuff seems to exist so that check_open can change the working directory (it doesn't on Linux) and we can change back to continue to use relative paths. Nothing ever changes the directory (at least not in the call paths of libxfs_init) so I think we can just rip it out. As a separate patch.
In the meantime, do you want me to resend with just the uncontroversial bits?
--D
Quoted 46 lines
libxfs/libxfs_priv.h int libxfs_zero_extent(struct xfs_inode *ip, xfs_fsblock_t start_fsb, · line 508
Quoted 2 lines
libxfs/libxfs_priv.h +2
-0+#define xfs_inode_set_cowblocks_tag (ip) do { } while (0) +#define xfs_inode_set_eofblocks_tag (ip) do { } while (0)This part looks fine.
libxfs/xfs_bmap.c xfs_bmap_validate_ret( · line 574
Quoted 5 lines
#else
#definexfs_bmap_check_leaf_extents(cur, ip, whichfork)do { } while (0)libxfs/xfs_bmap.c +1
-1-#define xfs_bmap_validate_ret (bno,len,flags,mval,onmap,nmap) +#define xfs_bmap_validate_ret (bno,len,flags,mval,onmap,nmap) do { } while (0)#endif /* DEBUG */ This as well.
Quoted 5 lines
repair/phase6.c +0
-0repair/phase6.c mark_standalone_inodes(xfs_mount_t *mp) · line 3099
Quoted 10 lines
repair/phase6.c +2
-2@@ -3092,11 +3092,11 @@ mark_standalone_inodes(xfs_mount_t *mp) irec =find_inode_rec (mp,XFS_INO_TO_AGNO (mp, mp->m_sb.sb_rsumino),XFS_INO_TO_AGINO (mp, mp->m_sb.sb_rsumino)); +ASSERT (irec !=NULL ); + offset =XFS_INO_TO_AGINO (mp, mp->m_sb.sb_rsumino) - irec->ino_startnum; -ASSERT (irec !=NULL ); -What's the point in even having this assert? Either we turn this
into something likeif (!irec) abort();or just let the code dereference it.
repair/phase6.c mark_standalone_inodes(xfs_mount_t *mp) · line 3101
Quoted 3 lines
repair/phase6.c +1
-1- imap.br_state = (rm_rec->rm_flags &XFS_RMAP_UNWRITTEN ? + imap.br_state = ((rm_rec->rm_flags &XFS_RMAP_UNWRITTEN ) ?XFS_EXT_UNWRITTEN :XFS_EXT_NORM );Looks fine.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 26 lines · Darrick J. Wong
Quoted 12 lines · Christoph Hellwig
Can you splits this up a bit to be more self explanatory, e.g. one patch per warning class or logical change?
db/check.c process_leaf_node_dir_v2_free( · line 3225
Quoted 6 lines
db/check.c +0
-2if (be32_to_cpu (free->hdr.nvalid) > maxent || -be32_to_cpu (free->hdr.nvalid) <0 ||be32_to_cpu (free->hdr.nused) > maxent || -be32_to_cpu (free->hdr.nused) <0 ||be32_to_cpu (free->hdr.nused) >be32_to_cpu (free->hdr.nvalid)) {
be32_to_cpureturns uint, so this makese sense.Quoted 12 lines
libxfs/init.c libxfs_init(libxfs_init_t *a) · line 268
Quoted 5 lines
libxfs/init.c +4
-1- (void )getcwd (curdir,MAXPATHLEN ); +if (!getcwd (curdir,MAXPATHLEN )) { +perror ("getcwd" ); +strcpy (curdir,"." ); + }All this chdir magic will need a lot more explanation. To me it seems like most of this is a left over from IRIX days where we could have device names relative to a volume. The proper fix probably is to just remove that whole thing - if we'd ever need to bring it back we should use openat relative to a dirfd instead.
Quoted 8 lines
Yeah, AFAICT all this getcwd/chdir stuff seems to exist so that
check_opencan change the working directory (it doesn't on Linux) and we can change back to continue to use relative paths. Nothing ever changes the directory (at least not in the call paths oflibxfs_init) so I think we can just rip it out. As a separate patch.In the meantime, do you want me to resend with just the uncontroversial bits?
The rest is all fine with me, sorry, I should have said that as well.
I agree with Christoph that splitting into logically grouped changes makes sense; I was going to let that slide, but it always has been and still is best practice, of course.
As for moving that ASSERT - yeah, at one point we said that if a null ptr deref is going to happen immediately after, the ASSERT is really of no value.
-Eric