[PATCH 1/4] fsstress: Fix wrong size argument to getcwd()

[PATCH 1/4] fsstress: Fix wrong size argument to getcwd()

[Rostislav Skudnov]

Fixes the following ASAN failure:

==11670==WARNING: AddressSanitizer failed to allocate 0xffffffffffffffff bytes
==11670==AddressSanitizer's allocator is terminating the process instead of returning 0

...

    #5 0x4bb230 in __interceptor_malloc /home/vak-local/3.9.1/release/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:65:10
    #6 0x7f97e6491405 in getcwd /build/glibc-6V9RKT/glibc-2.19/io/../sysdeps/unix/sysv/linux/getcwd.c:68
    #7 0x454691 in getcwd /home/vak-local/3.9.1/release/final/llvm.src/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:2822:15
    #8 0x4f765d in doproc /.../ltp/fsstress.c:933:12
    #9 0x4f5f54 in main /.../ltp/fsstress.c:581:5
---
 ltp/fsstress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 24c063e..96f48b1 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -965,7 +965,7 @@ doproc(void)
 		_exit(1);
 	}
 	top_ino = statbuf.st_ino;
-	homedir = getcwd(NULL, -1);
+	homedir = getcwd(NULL, 0);
 	seed += procid;
 	srandom(seed);
 	if (namerand)
-- 
2.1.4

[PATCH 2/4] writev_on_pagefault: Use ssize_t type for write(2) return value

[Rostislav Skudnov]

---
 src/writev_on_pagefault.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/writev_on_pagefault.c b/src/writev_on_pagefault.c
index 17dbfb7..eaa0c4b 100644
--- a/src/writev_on_pagefault.c
+++ b/src/writev_on_pagefault.c
@@ -43,7 +43,7 @@ void usage(char *progname)
 int main(int argc, char *argv[])
 {
 	int fd, i, c;
-	size_t ret;
+	ssize_t ret;
 	struct iovec *iov;
 	int pagesz = 4096;
 	char *data = NULL;
@@ -107,7 +107,7 @@ int main(int argc, char *argv[])
 	if (ret < 0)
 		perror("writev failed");
 	else
-		printf("wrote %d bytes\n", (int)ret);
+		printf("wrote %zd bytes\n", ret);
 
 	close(fd);
 	return 0;
-- 
2.1.4

[PATCH 3/4] holetest: Fix comparison of unsigned integer with 0

[Rostislav Skudnov]

---
 src/holetest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/holetest.c b/src/holetest.c
index 1939b35..e7ba123 100644
--- a/src/holetest.c
+++ b/src/holetest.c
@@ -205,7 +205,7 @@ int test_this(int fd, loff_t sz)
 			 */
 			fflush(stdout);
 			tid[i] = fork();
-			if (tid[i] < 0) {
+			if ((int64_t)tid[i] < 0) {
 				int j;
 
 				perror("fork");
-- 
2.1.4

[PATCH 4/4] bulkstat_unlink_test_modified: Remove extraneous if statement

[Rostislav Skudnov]

Fixes the following compiler warning:

bulkstat_unlink_test_modified.c:171:26: warning: equality comparison
with extraneous parentheses [-Wparentheses-equality]
                    if ((ret[i].bs_ino == inodelist[j])) {
                         ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
---
 src/bulkstat_unlink_test_modified.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/bulkstat_unlink_test_modified.c b/src/bulkstat_unlink_test_modified.c
index 6604992..eee7fd6 100644
--- a/src/bulkstat_unlink_test_modified.c
+++ b/src/bulkstat_unlink_test_modified.c
@@ -168,13 +168,12 @@ main(int argc, char *argv[])
 				 genlist[j]);
 			exit(1);
 		    }
-		    if ((ret[i].bs_ino == inodelist[j])) {
-			if ((genlist[j] + 1) != ret[i].bs_gen) {
-				/* oops, the new gen number is not 1 bigger than the old */
-				printf("Inode with old generation %d, new generation %d\n",
-				genlist[j], ret[i].bs_gen);
-				exit(1);
-			}
+		    if ((ret[i].bs_ino == inodelist[j]) &&
+			((genlist[j] + 1) != ret[i].bs_gen)) {
+			/* oops, the new gen number is not 1 bigger than the old */
+			printf("Inode with old generation %d, new generation %d\n",
+			genlist[j], ret[i].bs_gen);
+			exit(1);
 		    }
 		}
 	    }
-- 
2.1.4

Re: [PATCH 4/4] bulkstat_unlink_test_modified: Remove extraneous if statement

[Christoph Hellwig]

> bulkstat_unlink_test_modified.c:171:26: warning: equality comparison
> with extraneous parentheses [-Wparentheses-equality]
>                     if ((ret[i].bs_ino == inodelist[j])) {

Which the patch doesn't seem to actually remove..

> -		    if ((ret[i].bs_ino == inodelist[j])) {
> -			if ((genlist[j] + 1) != ret[i].bs_gen) {

module any reindentation this should be:

		if (ret[i].bs_ino == inodelist[j]) {
			if (genlist[j] + 1 != ret[i].bs_gen) {

alththough merging both conditionals also seems fine to me, as would
putting the ret array on the same side of both, e.g. something like:

		if (ret[i].bs_ino == inodelist[j] &&
		    ret[i].bs_gen != genlist[j]) {

Re: [PATCH 3/4] holetest: Fix comparison of unsigned integer with 0

[Christoph Hellwig]

On Thu, Oct 05, 2017 at 12:49:59PM +0000, Rostislav Skudnov wrote:
> ---
>  src/holetest.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/holetest.c b/src/holetest.c
> index 1939b35..e7ba123 100644
> --- a/src/holetest.c
> +++ b/src/holetest.c
> @@ -205,7 +205,7 @@ int test_this(int fd, loff_t sz)
>  			 */
>  			fflush(stdout);
>  			tid[i] = fork();
> -			if (tid[i] < 0) {
> +			if ((int64_t)tid[i] < 0) {

Just use the proper type for the return.  But I think this whole
file needs a redo - it tries to use tid both for storing a fork
return value and a pthread_t pointer.  If you only want a minimal
fix just introduce a int ret local variable.

Re: [PATCH 2/4] writev_on_pagefault: Use ssize_t type for write(2) return value

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/4] fsstress: Fix wrong size argument to getcwd()

[Christoph Hellwig]

Looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2] holetest: Use pid_t type for fork(2) return value

[Rostislav Skudnov]

---
Hello Christoph,

I think using uint64_t is fine since pthread_t is either a pointer or a ulong,
and pid_t is an int, so uint64_t acts as a "superset" of those types. Valid
PIDs are positive, so the difference in signedness should not be a problem.

I made a minimal patch as you suggested.
 src/holetest.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/holetest.c b/src/holetest.c
index 1939b35..ab582f5 100644
--- a/src/holetest.c
+++ b/src/holetest.c
@@ -199,22 +199,22 @@ int test_this(int fd, loff_t sz)
 			tid[i] = (uint64_t)t[i];
 			printf("INFO: thread %d created\n", i);
 		} else {
+			pid_t pid;
 			/*
 			 * Flush stdout before fork, otherwise some lines get
 			 * duplicated... ?!?!?
 			 */
 			fflush(stdout);
-			tid[i] = fork();
-			if (tid[i] < 0) {
+			pid = fork();
+			if (pid < 0) {
 				int j;
 
 				perror("fork");
 				for (j = 0; j < i; j++)
 					waitpid(tid[j], NULL, 0);
 				exit(21);
-			}
-			/* Child? */
-			if (!tid[i]) {
+			} else if (!pid) {
+				/* Child? */
 				void *ret;
 
 				if (use_wr[i])
@@ -223,6 +223,7 @@ int test_this(int fd, loff_t sz)
 					ret = pt_page_marker(&targs[i]);
 				exit(ret ? 1 : 0);
 			}
+			tid[i] = pid;
 			printf("INFO: process %d created\n", i);
 		}
 	}
-- 
2.1.4

[PATCH v2] bulkstat_unlink_test_modified: Remove extraneous if statement

[Rostislav Skudnov]

Fixes the following compiler warning:

bulkstat_unlink_test_modified.c:171:26: warning: equality comparison
with extraneous parentheses [-Wparentheses-equality]
                    if ((ret[i].bs_ino == inodelist[j])) {
                         ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
---
Updated according to your suggestion, removed extra parentheses.

 src/bulkstat_unlink_test_modified.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/bulkstat_unlink_test_modified.c b/src/bulkstat_unlink_test_modified.c
index 6604992..981d80c 100644
--- a/src/bulkstat_unlink_test_modified.c
+++ b/src/bulkstat_unlink_test_modified.c
@@ -168,13 +168,12 @@ main(int argc, char *argv[])
 				 genlist[j]);
 			exit(1);
 		    }
-		    if ((ret[i].bs_ino == inodelist[j])) {
-			if ((genlist[j] + 1) != ret[i].bs_gen) {
-				/* oops, the new gen number is not 1 bigger than the old */
-				printf("Inode with old generation %d, new generation %d\n",
-				genlist[j], ret[i].bs_gen);
-				exit(1);
-			}
+		    if (ret[i].bs_ino == inodelist[j] &&
+			ret[i].bs_gen != genlist[j] + 1) {
+			/* oops, the new gen number is not 1 bigger than the old */
+			printf("Inode with old generation %d, new generation %d\n",
+			genlist[j], ret[i].bs_gen);
+			exit(1);
 		    }
 		}
 	    }
-- 
2.1.4

Re: [PATCH v2] holetest: Use pid_t type for fork(2) return value

[Christoph Hellwig]

On Fri, Oct 06, 2017 at 08:19:46AM +0000, Rostislav Skudnov wrote:
> ---
> Hello Christoph,
> 
> I think using uint64_t is fine since pthread_t is either a pointer or a ulong,
> and pid_t is an int, so uint64_t acts as a "superset" of those types. Valid
> PIDs are positive, so the difference in signedness should not be a problem.

Well.  For one it loses any typeѕafety, second uint64_t is not
guaranteed to store a pointer (although in practice for the platforms
xfstests runs on it does), for that you'd need a uintptr_t.

Anyway, as a quick fix your patch below looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Although I still thing that code is extremtly badly written and using
proper types to store a pthread_t * vs pid would be a good idea.

Re: [PATCH v2] bulkstat_unlink_test_modified: Remove extraneous if statement

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>