xfsprogs: fix cut & paste error in xfs_fsr.c
PATCH 1/1 xfsprogs: fix cut & paste error in xfs_fsr.c
Commit:
7141fc xfsprogs: make fsr use mntinfo when there is no mntent
added an inadvertent "break;" to initallfs() after the call to find_mountpoint_check(); this is likely a cut & paste error from the call in find_mountpoint(), where we really *do* want to stop after the first one we find.
Fix that by removing the break, and fix the declaration-after-code.
Addresses-Coverity-Id: 1338431
---
I'll probably follow this with some other cleanups, but this is the functional fix AFAICT.
fsr/xfs_fsr.c +2 -3
@@ -433,12 +433,11 @@ initallfs(char *mtab) }while ( (mp =platform_mntent_next (&cursor)) !=NULL ) { +int rw =0 ; + mntp =find_mountpoint_check (&sb, mp, &ms);if (mntp ==NULL )continue ; -break ; - -int rw =0 ; if (strcmp(mp->mnt_type, MNTTYPE_XFS ) != 0 || stat64(mp->mnt_fsname, &sb) == -1 ||
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 9 lines · Eric Sandeen
Commit:
7141fc xfsprogs: make fsr use mntinfo when there is no mntent
added an inadvertent "break;" to
initallfs()after the call tofind_mountpoint_check(); this is likely a cut & paste error from the call infind_mountpoint(), where we really *do* want to stop after the first one we find.
Quoted 5 lines
Fix that by removing the break, and fix the declaration-after-code.
Addresses-Coverity-Id: 1338431
Signed-off-by Eric Sandeen <[email protected]>---
Actually, unless Jan's commit fixes something important for this release (and I don't think it does?) I'd just revert it; it's pretty chock full of outright errors and other weirdness.
For example:
initallfs(mtab);
struct stat64 sb;
mntp = find_mountpoint_check(&sb, mp, &ms);
and find_mounpoint_check() does:
if (S_ISDIR(sb->st_mode)) {so "sb" is never initialized on that path. Weird that Coverity didn't see it.
(I'm also not a fan of using "sb" and "mp" for things that aren't superblocks and xfs_mount_t's, but that's just style I guess...)
((and every caller passes an ms stat buffer into the function, which can be local to the function, as no caller uses it ...))
(((and there's whitespace damage)))
Jan said this patch "fell under the sofa" - I think it might need a bit more dusting off before it's ready to go... ;)
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 10 lines · Eric Sandeen
Commit:
7141fc xfsprogs: make fsr use mntinfo when there is no mntent
added an inadvertent "break;" to
initallfs()after the call tofind_mountpoint_check(); this is likely a cut & paste error from the call infind_mountpoint(), where we really *do* want to stop after the first one we find.Fix that by removing the break, and fix the declaration-after-code.
That's not the right fix - the find_mountpoint_check() shoul dnot be there at all. Patch to clean it all up is below.
-Dave.
Signature · 17 lines
Dave Chinner [email protected] fsr: clean up mountpoint checks From: Dave Chinner <[email protected]> Fix up a stray hunk of code from commit 7141fc5 ("xfsprogs: make fsr use mntinfo when there is no mntent") that coverity reported. Also clean up a couple of whitespace issues introduced with that commit, too. Addresses-Coverity-Id: 1338431 Signed-off-by: Dave Chinner <[email protected]> --- fsr/xfs_fsr.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)
fsr/xfs_fsr.c +5 -13
@@ -176,7 +176,6 @@ aborter(int unused)* here - the code that handles defragmentation of invidual files takes care * of that. */ -static char *find_mountpoint_check (struct stat64 *sb,struct mntent *t,struct stat64 *ms) {
@@ -200,9 +199,9 @@ find_mountpoint_check(struct stat64 *sb, struct mntent *t, struct stat64 *ms)return NULL ;/* -* Make sure the mountpoint given by mtab is accessible -* before using it. -*/ +* Make sure the mountpoint given by mtab is accessible +* before using it. +*/ if (stat64 (t->mnt_dir, &sb2) <0 )return NULL ; }
@@ -224,7 +223,7 @@ find_mountpoint(char *mtab, char *argname, struct stat64 *sb)exit (1 ); } -while ( (t =platform_mntent_next (&cursor)) !=NULL ) { +while ((t =platform_mntent_next (&cursor)) !=NULL ) { mntp =find_mountpoint_check (sb, t, &ms);if (mntp ==NULL )continue ;
@@ -409,12 +408,10 @@ static void initallfs(char *mtab) {struct mntent_cursor cursor; -char *mntp =NULL ;struct mntent *mp =NULL ;int mi;char *cp;struct stat64 sb; -struct stat64 ms;/* malloc a number of descriptors, increased later if needed */ if (!(fsbase = (fsdesc_t *)malloc (fsbufsize *sizeof (fsdesc_t)))) {
@@ -432,12 +429,7 @@ initallfs(char *mtab)exit (1 ); } -while ( (mp =platform_mntent_next (&cursor)) !=NULL ) { - mntp =find_mountpoint_check (&sb, mp, &ms); -if (mntp ==NULL ) -continue ; -break ; - +while ((mp =platform_mntent_next (&cursor)) !=NULL ) {int rw =0 ; if (strcmp(mp->mnt_type, MNTTYPE_XFS ) != 0 ||
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Eric Sandeen
Reviewed-byQuoted 10 lines · Eric Sandeen
Quoted 9 lines
Commit:
7141fc xfsprogs: make fsr use mntinfo when there is no mntent
added an inadvertent "break;" to
initallfs()after the call tofind_mountpoint_check(); this is likely a cut & paste error from the call infind_mountpoint(), where we really *do* want to stop after the first one we find.
Quoted 18 lines
Quoted 1 line
Fix that by removing the break, and fix the declaration-after-code.
That's not the right fix - the
find_mountpoint_check()shoul dnot be there at all. Patch to clean it all up is below.-Dave.
Signature · 11 lines
Dave Chinner [email protected] fsr: clean up mountpoint checks From: Dave Chinner <[email protected]> Fix up a stray hunk of code from commit 7141fc5 ("xfsprogs: make fsr use mntinfo when there is no mntent") that coverity reported. Also clean up a couple of whitespace issues introduced with that commit, too.
Um, ok, well it wasn't your misapplication, it was there in the original patch, and I wasn't sure of Jan's intent, having gone so far as to add local vars to support that placement. ;)
This seems fine, I guess I'll send another patch to make *ms local to find_mountpoint_check(), and rename some vars ("sb" and "mp" have their own expected meanings in xfsprogs)
-Eric
Quoted 58 lines
Addresses-Coverity-Id: 1338431
Signed-off-by Dave Chinner <[email protected]>--- fsr/xfs_fsr.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)fsr/xfs_fsr.c +4
-7@@ -176,7 +176,6 @@ aborter(int unused)* here - the code that handles defragmentation of invidual files takes care * of that. */ -static char *find_mountpoint_check (struct stat64 *sb,struct mntent *t,struct stat64 *ms) {@@ -200,9 +199,9 @@ find_mountpoint_check(struct stat64 *sb, struct mntent *t, struct stat64 *ms)return NULL ;/* -* Make sure the mountpoint given by mtab is accessible -* before using it. -*/ +* Make sure the mountpoint given by mtab is accessible +* before using it. +*/ if (stat64 (t->mnt_dir, &sb2) <0 )return NULL ; }@@ -224,7 +223,7 @@ find_mountpoint(char *mtab, char *argname, struct stat64 *sb)exit (1 ); } -while ( (t =platform_mntent_next (&cursor)) !=NULL ) { +while ((t =platform_mntent_next (&cursor)) !=NULL ) { mntp =find_mountpoint_check (sb, t, &ms);if (mntp ==NULL )continue ;@@ -409,12 +408,10 @@ static void initallfs(char *mtab) {struct mntent_cursor cursor; -char *mntp =NULL ;struct mntent *mp =NULL ;int mi;char *cp;struct stat64 sb; -struct stat64 ms;/* malloc a number of descriptors, increased later if needed */ if (!(fsbase = (fsdesc_t *)malloc (fsbufsize *sizeof (fsdesc_t)))) {@@ -432,12 +429,7 @@ initallfs(char *mtab)exit (1 ); }
fsr/xfs_fsr.c initallfs(char *mtab) · line 435
Quoted 10 lines
fsr/xfs_fsr.c +1
-6-while ( (mp =platform_mntent_next (&cursor)) !=NULL ) { - mntp =find_mountpoint_check (&sb, mp, &ms); -if (mntp ==NULL ) -continue ; -break ; - +while ((mp =platform_mntent_next (&cursor)) !=NULL ) {int rw =0 ; if (strcmp(mp->mnt_type, MNTTYPE_XFS ) != 0 ||
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 29 lines · Eric Sandeen
Quoted 20 lines · Eric Sandeen
Quoted 10 lines
Commit:
7141fc xfsprogs: make fsr use mntinfo when there is no mntent
added an inadvertent "break;" to
initallfs()after the call tofind_mountpoint_check(); this is likely a cut & paste error from the call infind_mountpoint(), where we really *do* want to stop after the first one we find.Fix that by removing the break, and fix the declaration-after-code.
That's not the right fix - the
find_mountpoint_check()shoul dnot be there at all. Patch to clean it all up is below.-Dave.
--
Dave Chinner
[email protected]Quoted 8 lines
fsr: clean up mountpoint checks
From: Dave Chinner <[email protected]>
Fix up a stray hunk of code from commit 7141fc5 ("xfsprogs: make fsr use mntinfo when there is no mntent") that coverity reported. Also clean up a couple of whitespace issues introduced with that commit, too.
Quoted 9 lines
Um, ok, well it wasn't your misapplication, it was there in the original patch,
and I wasn't sure of Jan's intent, having gone so far as to add local vars to support that placement. ;)This seems fine, I guess I'll send another patch to make *ms local to
find_mountpoint_check(), and rename some vars ("sb" and "mp" have their own expected meanings in xfsprogs)
Hmmm, sorry about the mess... :-( I will look on the patch again too. Speaking about Coverity, is there a way I can get to it and use it myself? (Maybe it may be better to continue this off list - from what I heard, Coverity guys don't like to be talk about publicly...)
Cheers,
Jan
Signature · 2 lines
Jan Tulak [email protected] / [email protected]
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 6 lines · Jan Tulak
Hmmm, sorry about the mess... :-( I will look on the patch again too. Speaking about Coverity, is there a way I can get to it and use it myself? (Maybe it may be better to continue this off list - from what I heard, Coverity guys don't like to be talk about publicly...)
Cheers, Jan
So, we have xfsprogs & xfsdump in the upstream "Coverity Scan" project, which is free for open source projects.
Nah, it's not secret, although access to the defect database is controlled, out of security concerns I guess - I sent you an invite.
Thanks,
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/1 xfsprogs: tidy up xfs_fsr.c
In reply to Dave ChinnerMake the mountpoint stat structure "ms" local to find_mountpoint_check(), no caller uses it. And remove 3rd "sb2" statbuf in that same function, we can just re-use ms.
rename "struct mntent *mp" to "struct mntent *mnt" - "mp" has its own common meaning in xfsprogs.
And a couple more minor whitespace removals.
---
fsr/xfs_fsr.c +20 -23
@@ -177,44 +177,41 @@ aborter(int unused)* of that. */ static char * -find_mountpoint_check (struct stat64 *sb,struct mntent *t,struct stat64 *ms) +find_mountpoint_check (struct stat64 *sb,struct mntent *t) { +struct stat64 ms; +if (S_ISDIR (sb->st_mode)) {/* mount point */ -if (stat64 (t->mnt_dir, ms) <0 ) +if (stat64 (t->mnt_dir, &ms) <0 )return NULL ; -if (sb->st_ino != ms->st_ino) +if (sb->st_ino != ms.st_ino)return NULL ; -if (sb->st_dev != ms->st_dev) +if (sb->st_dev != ms.st_dev)return NULL ;if (strcmp (t->mnt_type,MNTTYPE_XFS ) !=0 )return NULL ; }else {/* device */ -struct stat64 sb2; - -if (stat64 (t->mnt_fsname, ms) <0 ) +if (stat64 (t->mnt_fsname, &ms) <0 )return NULL ; -if (sb->st_rdev != ms->st_rdev) +if (sb->st_rdev != ms.st_rdev)return NULL ;if (strcmp (t->mnt_type,MNTTYPE_XFS ) !=0 )return NULL ; -/* * Make sure the mountpoint given by mtab is accessible * before using it. */ -if (stat64 (t->mnt_dir, &sb2) <0 ) +if (stat64 (t->mnt_dir, &ms) <0 )return NULL ; }return t->mnt_dir; - }static char *find_mountpoint (char *mtab,char *argname,struct stat64 *sb) {struct mntent_cursor cursor; -struct stat64 ms;struct mntent *t =NULL ;char *mntp =NULL ;
@@ -224,7 +221,7 @@ find_mountpoint(char *mtab, char *argname, struct stat64 *sb) }while ((t =platform_mntent_next (&cursor)) !=NULL ) { - mntp =find_mountpoint_check (sb, t, &ms); + mntp =find_mountpoint_check (sb, t);if (mntp ==NULL )continue ;break ;
@@ -408,7 +405,7 @@ static void initallfs(char *mtab) {struct mntent_cursor cursor; -struct mntent *mp =NULL ; +struct mntent *mnt=NULL ;int mi;char *cp;struct stat64 sb;
@@ -429,15 +426,15 @@ initallfs(char *mtab)exit (1 ); } -while ((mp =platform_mntent_next (&cursor)) !=NULL ) { +while ((mnt =platform_mntent_next (&cursor)) !=NULL ) {int rw =0 ; -if (strcmp (mp->mnt_type,MNTTYPE_XFS ) !=0 || -stat64 (mp->mnt_fsname, &sb) ==-1 || +if (strcmp (mnt->mnt_type,MNTTYPE_XFS ) !=0 || +stat64 (mnt->mnt_fsname, &sb) ==-1 || !S_ISBLK (sb.st_mode))continue ; - cp =strtok (mp->mnt_opts,"," ); + cp =strtok (mnt->mnt_opts,"," ); do { if (strcmp("rw", cp) == 0) rw++;
@@ -445,7 +442,7 @@ initallfs(char *mtab)if (rw ==0 ) {if (dflag)fsrprintf (_ ("Skipping %s: not mounted rw\n" ), - mp->mnt_fsname); + mnt->mnt_fsname);continue ; }
@@ -465,15 +462,15 @@ initallfs(char *mtab) fs = (fsbase + mi);/* Needed ? */ } - fs->dev =strdup (mp->mnt_fsname); - fs->mnt =strdup (mp->mnt_dir); + fs->dev =strdup (mnt->mnt_fsname); + fs->mnt =strdup (mnt->mnt_dir);if (fs->dev ==NULL ) { -fsrprintf (_ ("strdup(%s) failed\n" ), mp->mnt_fsname); +fsrprintf (_ ("strdup(%s) failed\n" ), mnt->mnt_fsname);exit (1 ); }if (fs->mnt ==NULL ) { -fsrprintf (_ ("strdup(%s) failed\n" ), mp->mnt_dir); +fsrprintf (_ ("strdup(%s) failed\n" ), mnt->mnt_dir);exit (1 ); } mi++;
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs