Skip to contentSkip to conversation

xfsprogs: fix cut & paste error in xfs_fsr.c

PATCH In review Eric Sandeen 4 participants 7 messages Nov 10, 2015 – Nov 11, 2015

lore
Share & tools
lore
#0 lore raw

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
index b902acc..9332c57 100644
@@ -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

#1 lore raw
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 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.

Quoted 5 lines

Fix that by removing the break, and fix the declaration-after-code.

Addresses-Coverity-Id: 1338431

---

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

#2 lore raw
In reply to Eric Sandeen
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 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.

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
index b902acc..2887ceb 100644
@@ -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

author Reviewed-by
#3 lore raw
Quoted 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 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.

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

---
 fsr/xfs_fsr.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)
fsr/xfs_fsr.c +4 -7
index b902acc..2887ceb 100644
@@ -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

#4 lore raw
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 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.

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 b​etter 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

#5 lore raw
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 b​etter 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

#6 lore raw

PATCH 1/1 xfsprogs: tidy up xfs_fsr.c

In reply to Dave Chinner

Make 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
index 2887ceb..d5962f7 100644
@@ -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