filefrag: fix minor issues with 29758d2

PATCH Superseded Eric Sandeen 4 participants 9 messages May 29, 2014 – Jun 2, 2014

lore

Share & tools

lore

Eric Sandeen

author

2014-05-29 17:26 UTC #0 lore raw

PATCH 1/1 filefrag: fix minor issues with 29758d2

29758d2 filefrag: exit with error code if an error is hit

introduced a couple errors; in one case it missed returning a value, and in the other used a test where it needed an assignment.

Signed-off-by Eric Sandeen <[email protected]>

---

misc/filefrag.c +3 -2

index 37c4416..b2826ad 100644
@@ -387,8 +387,9 @@ static int frag_report(const char *filename)
 	if (last_device != st.st_dev) {
 		if (fstatfs(fd, &fsinfo) < 0) {
 			close(fd);
+			rc = -errno;
 			perror("fstatfs");
-			return;
+			return rc;
 		}
 		if (verbose)
 			printf("Filesystem type is: %lx\n",

@@ -556,7 +557,7 @@ int main(int argc, char**argv)
 		int rc2 = frag_report(*cpp);
 
 		if (rc2 < 0 && rc == 0)
-			rc == rc2;
+			rc = rc2;
 	}
 
 	return rc;

Eric Sandeen

author

2014-05-29 17:31 UTC #1 lore raw

Quoted 14 lines · Eric Sandeen

29758d2 filefrag: exit with error code if an error is hit
introduced a couple errors; in one case it missed returning a value, and in the other used a test where it needed an assignment.
Signed-off-by Eric Sandeen <[email protected]>
---
misc/filefrag.c +0 -0
index 37c4416..b2826ad 100644

misc/filefrag.c static int frag_report(const char *filename) · line 393

Quoted 8 lines

misc/filefrag.c +2 -1

@@ -387,8 +387,9 @@ static int frag_report(const char *filename)
 	if (last_device != st.st_dev) {
 		if (fstatfs(fd, &fsinfo) < 0) {
 			close(fd);
+			rc = -errno;
 			perror("fstatfs");
-			return;
+			return rc;
 		}

And as an aside, here and other places where we do:

if (syscall() < 0) {
	close (fd);
	rc = -errno;
	perror("syscall");
	return rc;
}

we actually run the risk of close failing, and getting errno/perror from THAT, but *shrug* for now...

-Eric

Quoted 17 lines

misc/filefrag.c +1 -2
 		if (verbose)
 			printf("Filesystem type is: %lx\n",
@@ -556,7 +557,7 @@ int main(int argc, char**argv)
 		int rc2 = frag_report(*cpp);
 
 		if (rc2 < 0 && rc == 0)
-			rc == rc2;
+			rc = rc2;
 	}
 
 	return rc;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lukáš Czerner

2014-05-29 17:35 UTC #2 lore raw

In reply to Eric Sandeen

Quoted 11 lines · Eric Sandeen

Date: Thu, 29 May 2014 12:25:17 -0500
From: Eric Sandeen <[email protected]>
To: ext4 development <[email protected]>
Cc: Andreas Dilger <[email protected]>
Subject: [PATCH] filefrag: fix minor issues with 29758d2
29758d2 filefrag: exit with error code if an error is hit
introduced a couple errors; in one case it missed returning a value, and in the other used a test where it needed an assignment.

Looks good, although I wonder if we can just return -errno or goto out_close ? (same with fstat failure).

Thanks!
-Lukas

Quoted 34 lines

Signed-off-by Eric Sandeen <[email protected]>

---

misc/filefrag.c +3 -3

index 37c4416..b2826ad 100644
@@ -387,8 +387,9 @@ static int frag_report(const char *filename)
 	if (last_device != st.st_dev) {
 		if (fstatfs(fd, &fsinfo) < 0) {
 			close(fd);
+			rc = -errno;
 			perror("fstatfs");
-			return;
+			return rc;
 		}
 		if (verbose)
 			printf("Filesystem type is: %lx\n",

@@ -556,7 +557,7 @@ int main(int argc, char**argv)
 		int rc2 = frag_report(*cpp);
 
 		if (rc2 < 0 && rc == 0)
-			rc == rc2;
+			rc = rc2;
 	}
 
 	return rc;
 
--

To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Sandeen

author

2014-05-29 17:43 UTC #3 lore raw

Quoted 8 lines · Lukáš Czerner

Quoted 6 lines · Eric Sandeen
Date: Thu, 29 May 2014 12:25:17 -0500
From: Eric Sandeen <[email protected]>
To: ext4 development <[email protected]>
Cc: Andreas Dilger <[email protected]>
Subject: [PATCH] filefrag: fix minor issues with 29758d2

Quoted 8 lines

Quoted 5 lines
29758d2 filefrag: exit with error code if an error is hit
introduced a couple errors; in one case it missed returning a value, and in the other used a test where it needed an assignment.
Looks good, although I wonder if we can just return -errno or goto out_close ? (same with fstat failure).

Perhaps; I was just following the style of the error handling block before it, rather than refactoring w/ gotos and such. :)

Going for the "small fix."

-Eric

Quoted 30 lines

Thanks!
-Lukas

Quoted 27 lines

Signed-off-by Eric Sandeen <[email protected]>

---

misc/filefrag.c +3 -2

index 37c4416..b2826ad 100644
@@ -387,8 +387,9 @@ static int frag_report(const char *filename)
 	if (last_device != st.st_dev) {
 		if (fstatfs(fd, &fsinfo) < 0) {
 			close(fd);
+			rc = -errno;
 			perror("fstatfs");
-			return;
+			return rc;
 		}
 		if (verbose)
 			printf("Filesystem type is: %lx\n",

@@ -556,7 +557,7 @@ int main(int argc, char**argv)
 		int rc2 = frag_report(*cpp);
 
 		if (rc2 < 0 && rc == 0)
-			rc == rc2;
+			rc = rc2;
 	}

Quoted 7 lines

Quoted 7 lines
misc/filefrag.c +0 -1
 	return rc;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Sandeen

author

2014-05-29 17:42 UTC #4 lore raw

PATCH v2 1/1 filefrag: fix issues with 29758d2

In reply to Eric Sandeen

29758d2 filefrag: exit with error code if an error is hit

introduced a couple errors; in one case it missed returning a value, and in the other used a test where it needed an assignment.

Also fix a precedence problem with:

	if (fe_flags & mask == 0)

which is equivalent to:

	if (fe_flags & (mask == 0))

but we need:

	if ((fe_flags & mask) == 0)

Signed-off-by Eric Sandeen <[email protected]>

---

misc/filefrag.c +4 -3

index 37c4416..1d05f47 100644
@@ -175,7 +175,7 @@ static void print_extent_info(struct fiemap_extent *fm_extent, int cur_ex,
 	for (mask = 1; fe_flags != 0 && mask != 0; mask <<= 1) {
 		char hex[6];
 
-		if (fe_flags & mask == 0)
+		if ((fe_flags & mask) == 0)
 			continue;
 		sprintf(hex, "%#04x,", mask);
 		print_flag(&fe_flags, mask, flags, hex);

@@ -387,8 +387,9 @@ static int frag_report(const char *filename)
 	if (last_device != st.st_dev) {
 		if (fstatfs(fd, &fsinfo) < 0) {
 			close(fd);
+			rc = -errno;
 			perror("fstatfs");
-			return;
+			return rc;
 		}
 		if (verbose)
 			printf("Filesystem type is: %lx\n",

@@ -556,7 +557,7 @@ int main(int argc, char**argv)
 		int rc2 = frag_report(*cpp);
 
 		if (rc2 < 0 && rc == 0)
-			rc == rc2;
+			rc = rc2;
 	}
 
 	return rc;

Andreas Dilger

2014-05-30 06:45 UTC #5 lore raw

Re: [PATCH V2] filefrag: fix issues with 29758d2

Quoted 10 lines · Eric Sandeen

29758d2 filefrag: exit with error code if an error is hit
introduced a couple errors; in one case it missed returning a value, and in the other used a test where it needed an assignment.
Also fix a precedence problem with:
	if (fe_flags & mask == 0)

Quoted 7 lines

which is equivalent to:
	if (fe_flags & (mask == 0))
but we need:
	if ((fe_flags & mask) == 0)

Strange. I'm pretty sure I fixed this, because it was causing every flag to be printed, but I guess I sent the wrong version or something. Thanks for fixing this.

Quoted 14 lines

misc/filefrag.c +1 -1

index 37c4416..1d05f47 100644
@@ -175,7 +175,7 @@ static void print_extent_info(struct fiemap_extent *fm_extent, int cur_ex,
 	for (mask = 1; fe_flags != 0 && mask != 0; mask <<= 1) {
 		char hex[6];
 
-		if (fe_flags & mask == 0)
+		if ((fe_flags & mask) == 0)
 			continue;
 		sprintf(hex, "%#04x,", mask);
 		print_flag(&fe_flags, mask, flags, hex);

misc/filefrag.c static int frag_report(const char *filename) · line 390

Quoted 4 lines

misc/filefrag.c +1 -0

@@ -387,8 +387,9 @@ static int frag_report(const char *filename)
 	if (last_device != st.st_dev) {
 		if (fstatfs(fd, &fsinfo) < 0) {
 			close(fd);
+			rc = -errno;

Would be better to move this above close(), instead of introducing another case of the potential error you mentioned.

Cheers, Andreas

Quoted 17 lines

misc/filefrag.c +2 -3

 			perror("fstatfs");
-			return;
+			return rc;
 		}
 		if (verbose)
 			printf("Filesystem type is: %lx\n",

@@ -556,7 +557,7 @@ int main(int argc, char**argv)
 		int rc2 = frag_report(*cpp);
 
 		if (rc2 < 0 && rc == 0)
-			rc == rc2;
+			rc = rc2;
 	}
 
 	return rc;
 
--

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Cheers, Andreas

Eric Sandeen

author

2014-05-30 18:10 UTC #6 lore raw

PATCH v3 1/1 filefrag: filefrag: fix issues with 29758d2

In reply to Eric Sandeen

29758d2 filefrag: exit with error code if an error is hit

introduced a couple errors; in one case it missed returning a value, and possibly picked up errno from (unchecked) close(), and in the other used a test where it needed an assignment. So capture the error, move perror() directly after the failed call in both cases, and fix the assignment.

Also fix a precedence problem with:

	if (fe_flags & mask == 0)

which is equivalent to:

	if (fe_flags & (mask == 0))

but we need:

	if ((fe_flags & mask) == 0)

Signed-off-by Eric Sandeen <[email protected]>

---

misc/filefrag.c +6 -5

index 37c4416..820821b 100644
@@ -175,7 +175,7 @@ static void print_extent_info(struct fiemap_extent *fm_extent, int cur_ex,
 	for (mask = 1; fe_flags != 0 && mask != 0; mask <<= 1) {
 		char hex[6];
 
-		if (fe_flags & mask == 0)
+		if ((fe_flags & mask) == 0)
 			continue;
 		sprintf(hex, "%#04x,", mask);
 		print_flag(&fe_flags, mask, flags, hex);

@@ -378,17 +378,18 @@ static int frag_report(const char *filename)
 #else
 	if (fstat(fd, &st) < 0) {
 #endif
-		close(fd);
 		rc = -errno;
 		perror("stat");
+		close(fd);
 		return rc;
 	}
 
 	if (last_device != st.st_dev) {
 		if (fstatfs(fd, &fsinfo) < 0) {
-			close(fd);
+			rc = -errno;
 			perror("fstatfs");
-			return;
+			close(fd);
+			return rc;
 		}
 		if (verbose)
 			printf("Filesystem type is: %lx\n",

@@ -556,7 +557,7 @@ int main(int argc, char**argv)
 		int rc2 = frag_report(*cpp);
 
 		if (rc2 < 0 && rc == 0)
-			rc == rc2;
+			rc = rc2;
 	}
 
 	return rc;

Andreas Dilger

Reviewed-by on v3 1/1

2014-05-30 21:03 UTC #7 lore raw

Re: [PATCH V3] filefrag: filefrag: fix issues with 29758d2

Quoted 14 lines · Eric Sandeen

29758d2 filefrag: exit with error code if an error is hit
introduced a couple errors; in one case it missed returning a value, and possibly picked up errno from (unchecked) close(), and in the other used a test where it needed an assignment. So capture the error, move perror() directly after the failed call in both cases, and fix the assignment.
Also fix a precedence problem with:
	if (fe_flags & mask == 0)
which is equivalent to:

Quoted 7 lines

	if (fe_flags & (mask == 0))
but we need:
	if ((fe_flags & mask) == 0)
Signed-off-by Eric Sandeen <[email protected]>

Looks good:

Reviewed-by Andreas Dilger <[email protected]>

Quoted 39 lines

---

misc/filefrag.c +5 -4

index 37c4416..820821b 100644
@@ -175,7 +175,7 @@ static void print_extent_info(struct fiemap_extent *fm_extent, int cur_ex,
 	for (mask = 1; fe_flags != 0 && mask != 0; mask <<= 1) {
 		char hex[6];
 
-		if (fe_flags & mask == 0)
+		if ((fe_flags & mask) == 0)
 			continue;
 		sprintf(hex, "%#04x,", mask);
 		print_flag(&fe_flags, mask, flags, hex);

@@ -378,17 +378,18 @@ static int frag_report(const char *filename)
 #else
 	if (fstat(fd, &st) < 0) {
 #endif
-		close(fd);
 		rc = -errno;
 		perror("stat");
+		close(fd);
 		return rc;
 	}
 
 	if (last_device != st.st_dev) {
 		if (fstatfs(fd, &fsinfo) < 0) {
-			close(fd);
+			rc = -errno;
 			perror("fstatfs");
-			return;
+			close(fd);
+			return rc;
 		}
 		if (verbose)
 			printf("Filesystem type is: %lx\n",

misc/filefrag.c int main(int argc, char**argv) · line 563

Quoted 8 lines

misc/filefrag.c +1 -1

@@ -556,7 +557,7 @@ int main(int argc, char**argv)
 		int rc2 = frag_report(*cpp);
 
 		if (rc2 < 0 && rc == 0)
-			rc == rc2;
+			rc = rc2;
 	}
 
 	return rc;

Cheers, Andreas

Theodore Ts'o

Applied

2014-06-02 01:28 UTC #8 lore raw

Re: [PATCH V3] filefrag: filefrag: fix issues with 29758d2

Thanks, applied.

- Ted