* [PATCH] misc: fix more stupid compiler warnings
@ 2017-09-01 22:21 Darrick J. Wong
2017-09-03 3:41 ` Eric Sandeen
2017-09-03 7:24 ` Christoph Hellwig
0 siblings, 2 replies; 5+ messages in thread
From: Darrick J. Wong @ 2017-09-01 22:21 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
Fix more compiler warnings about pointless checks, unchecked return
values, brace problems, and missing parentheses.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
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(-)
diff --git a/db/check.c b/db/check.c
index 12a0aed..6076540 100644
--- a/db/check.c
+++ b/db/check.c
@@ -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)
diff --git a/libxfs/init.c b/libxfs/init.c
index d77a9e6..2479220 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -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,
diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index 19bdbdb..94804fe 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -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
diff --git a/libxfs/xfs_bmap.c b/libxfs/xfs_bmap.c
index 31a631a..ab5853d 100644
--- a/libxfs/xfs_bmap.c
+++ b/libxfs/xfs_bmap.c
@@ -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 */
/*
diff --git a/repair/phase6.c b/repair/phase6.c
index b051a44..f360aed 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -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) {
diff --git a/repair/rmap.c b/repair/rmap.c
index 9710565..07d1ba8 100644
--- a/repair/rmap.c
+++ b/repair/rmap.c
@@ -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,
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] misc: fix more stupid compiler warnings
2017-09-01 22:21 [PATCH] misc: fix more stupid compiler warnings Darrick J. Wong
@ 2017-09-03 3:41 ` Eric Sandeen
2017-09-03 7:24 ` Christoph Hellwig
1 sibling, 0 replies; 5+ messages in thread
From: Eric Sandeen @ 2017-09-03 3:41 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: xfs
On 9/1/17 5:21 PM, Darrick J. Wong wrote:
> Fix more compiler warnings about pointless checks, unchecked return
> values, brace problems, and missing parentheses.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> 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(-)
>
> diff --git a/db/check.c b/db/check.c
> index 12a0aed..6076540 100644
> --- a/db/check.c
> +++ b/db/check.c
> @@ -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)
> diff --git a/libxfs/init.c b/libxfs/init.c
> index d77a9e6..2479220 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -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?
> 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);
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] misc: fix more stupid compiler warnings
2017-09-01 22:21 [PATCH] misc: fix more stupid compiler warnings Darrick J. Wong
2017-09-03 3:41 ` Eric Sandeen
@ 2017-09-03 7:24 ` Christoph Hellwig
2017-09-04 0:22 ` Darrick J. Wong
1 sibling, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2017-09-03 7:24 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Eric Sandeen, xfs
Can you splits this up a bit to be more self explanatory, e.g.
one patch per warning class or logical change?
> 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)) {
be32_to_cpu returns uint, so this makese sense.
> - (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.
> +#define xfs_inode_set_cowblocks_tag(ip) do { } while (0)
> +#define xfs_inode_set_eofblocks_tag(ip) do { } while (0)
This part looks fine.
> #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 */
This as well.
> diff --git a/repair/phase6.c b/repair/phase6.c
> index b051a44..f360aed 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -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.
> - 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] misc: fix more stupid compiler warnings
2017-09-03 7:24 ` Christoph Hellwig
@ 2017-09-04 0:22 ` Darrick J. Wong
2017-09-04 1:01 ` Eric Sandeen
0 siblings, 1 reply; 5+ messages in thread
From: Darrick J. Wong @ 2017-09-04 0:22 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Eric Sandeen, xfs
On Sun, Sep 03, 2017 at 12:24:33AM -0700, Christoph Hellwig wrote:
> Can you splits this up a bit to be more self explanatory, e.g.
> one patch per warning class or logical change?
>
> > 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)) {
>
> be32_to_cpu returns uint, so this makese sense.
>
> > - (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
> > +#define xfs_inode_set_cowblocks_tag(ip) do { } while (0)
> > +#define xfs_inode_set_eofblocks_tag(ip) do { } while (0)
>
> This part looks fine.
>
> > #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 */
>
> This as well.
>
> > diff --git a/repair/phase6.c b/repair/phase6.c
> > index b051a44..f360aed 100644
> > --- a/repair/phase6.c
> > +++ b/repair/phase6.c
> > @@ -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.
>
> > - 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] misc: fix more stupid compiler warnings
2017-09-04 0:22 ` Darrick J. Wong
@ 2017-09-04 1:01 ` Eric Sandeen
0 siblings, 0 replies; 5+ messages in thread
From: Eric Sandeen @ 2017-09-04 1:01 UTC (permalink / raw)
To: Darrick J. Wong, Christoph Hellwig; +Cc: xfs
On 9/3/17 7:22 PM, Darrick J. Wong wrote:
> On Sun, Sep 03, 2017 at 12:24:33AM -0700, Christoph Hellwig wrote:
>> Can you splits this up a bit to be more self explanatory, e.g.
>> one patch per warning class or logical change?
>>
>>> 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)) {
>>
>> be32_to_cpu returns uint, so this makese sense.
>>
>>> - (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?
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-09-04 1:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-01 22:21 [PATCH] misc: fix more stupid compiler warnings Darrick J. Wong
2017-09-03 3:41 ` Eric Sandeen
2017-09-03 7:24 ` Christoph Hellwig
2017-09-04 0:22 ` Darrick J. Wong
2017-09-04 1:01 ` Eric Sandeen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.