[PATCH] xfsprogs: fix cut & paste error in xfs_fsr.c

[PATCH] xfsprogs: fix cut & paste error in xfs_fsr.c

[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.

Addresses-Coverity-Id: 1338431
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

I'll probably follow this with some other cleanups, but this is
the functional fix AFAICT.

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index b902acc..9332c57 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: fix cut & paste error in xfs_fsr.c

[Eric Sandeen]

On 11/10/15 12:21 PM, Eric Sandeen wrote:
> 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
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: fix cut & paste error in xfs_fsr.c

[Dave Chinner]

On Tue, Nov 10, 2015 at 12:21:10PM -0600, Eric Sandeen wrote:
> 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
david@fromorbit.com

fsr: clean up mountpoint checks

From: Dave Chinner <dchinner@redhat.com>

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 <dchinner@redhat.com>
---
 fsr/xfs_fsr.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index b902acc..2887ceb 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: fix cut & paste error in xfs_fsr.c

[Eric Sandeen]

> On Tue, Nov 10, 2015 at 12:21:10PM -0600, Eric Sandeen wrote:
>> 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
> david@fromorbit.com
> 
> fsr: clean up mountpoint checks
> 
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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)

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

-Eric

> Addresses-Coverity-Id: 1338431
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fsr/xfs_fsr.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index b902acc..2887ceb 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH] xfsprogs: tidy up xfs_fsr.c

[Eric Sandeen]

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.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 2887ceb..d5962f7 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: fix cut & paste error in xfs_fsr.c

[Jan Tulak]

[-- Attachment #1.1: Type: text/plain, Size: 1840 bytes --]

On Tue, Nov 10, 2015 at 9:35 PM, Eric Sandeen <sandeen@redhat.com> wrote:

> > On Tue, Nov 10, 2015 at 12:21:10PM -0600, Eric Sandeen wrote:
> >> 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
> > david@fromorbit.com
> >
> > fsr: clean up mountpoint checks
> >
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > 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)
>

​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


-- 
Jan Tulak
jtulak@redhat.com / jan@tulak.me

[-- Attachment #1.2: Type: text/html, Size: 3425 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: fix cut & paste error in xfs_fsr.c

[Eric Sandeen]

On 11/11/15 7:13 AM, Jan Tulak wrote:

> ​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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs