[PATCH v3 0/7] Expand selftest utils

[PATCH v3 0/7] Expand selftest utils

[Benjamin Gray]

Started this when writing tests for a feature I'm working on, needing a way to
read/write numbers to system files. After writing some utils to safely handle
file IO and parsing, I realised I'd made the ~6th file read/write implementation
and only(?) number parser that checks all the failure modes when expecting to
parse a single number from a file.

So these utils ended up becoming this series. I also modified some other test
utils I came across while doing so. My understanding is selftests are not expected
to be backported, so I wasn't concerned about only introducing new utils and leaving
the existing implementations be.

V3:	* Add reviewed-by from previous version
	* Fix write(2) call to include creation mode

Benjamin Gray (7):
  selftests/powerpc: Use mfspr/mtspr macros
  selftests/powerpc: Add ptrace setup_core_pattern() null-terminator
  selftests/powerpc: Add generic read/write file util
  selftests/powerpc: Add read/write debugfs file, int
  selftests/powerpc: Parse long/unsigned long value safely
  selftests/powerpc: Add {read,write}_{long,ulong}
  selftests/powerpc: Add automatically allocating read_file

 tools/testing/selftests/powerpc/dscr/dscr.h   |  56 +---
 .../selftests/powerpc/dscr/dscr_sysfs_test.c  |  23 +-
 .../testing/selftests/powerpc/include/utils.h |  18 +-
 .../selftests/powerpc/nx-gzip/gzfht_test.c    |  52 +--
 tools/testing/selftests/powerpc/pmu/lib.c     |  35 +-
 .../selftests/powerpc/ptrace/core-pkey.c      |  28 +-
 .../selftests/powerpc/ptrace/ptrace-hwbreak.c |   6 +-
 .../testing/selftests/powerpc/ptrace/ptrace.h |   5 +-
 .../selftests/powerpc/security/entry_flush.c  |  12 +-
 .../selftests/powerpc/security/flush_utils.c  |   3 +-
 .../selftests/powerpc/security/rfi_flush.c    |  12 +-
 .../powerpc/security/uaccess_flush.c          |  18 +-
 .../selftests/powerpc/syscalls/Makefile       |   2 +-
 .../selftests/powerpc/syscalls/rtas_filter.c  |  80 +----
 tools/testing/selftests/powerpc/utils.c       | 314 ++++++++++++++----
 15 files changed, 341 insertions(+), 323 deletions(-)


base-commit: 247f34f7b80357943234f93f247a1ae6b6c3a740
--
2.38.1

[PATCH v3 1/7] selftests/powerpc: Use mfspr/mtspr macros

[Benjamin Gray]

No need to write inline asm for mtspr/mfspr, we have macros for this
in reg.h

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
---
 tools/testing/selftests/powerpc/dscr/dscr.h     | 17 +++++------------
 .../selftests/powerpc/ptrace/ptrace-hwbreak.c   |  6 ++----
 tools/testing/selftests/powerpc/ptrace/ptrace.h |  5 +----
 .../selftests/powerpc/security/flush_utils.c    |  3 ++-
 4 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/powerpc/dscr/dscr.h b/tools/testing/selftests/powerpc/dscr/dscr.h
index 13e9b9e28e2c..b703714e7d98 100644
--- a/tools/testing/selftests/powerpc/dscr/dscr.h
+++ b/tools/testing/selftests/powerpc/dscr/dscr.h
@@ -23,6 +23,7 @@
 #include <sys/stat.h>
 #include <sys/wait.h>
 
+#include "reg.h"
 #include "utils.h"
 
 #define THREADS		100	/* Max threads */
@@ -41,31 +42,23 @@
 /* Prilvilege state DSCR access */
 inline unsigned long get_dscr(void)
 {
-	unsigned long ret;
-
-	asm volatile("mfspr %0,%1" : "=r" (ret) : "i" (SPRN_DSCR_PRIV));
-
-	return ret;
+	return mfspr(SPRN_DSCR_PRIV);
 }
 
 inline void set_dscr(unsigned long val)
 {
-	asm volatile("mtspr %1,%0" : : "r" (val), "i" (SPRN_DSCR_PRIV));
+	mtspr(SPRN_DSCR_PRIV, val);
 }
 
 /* Problem state DSCR access */
 inline unsigned long get_dscr_usr(void)
 {
-	unsigned long ret;
-
-	asm volatile("mfspr %0,%1" : "=r" (ret) : "i" (SPRN_DSCR));
-
-	return ret;
+	return mfspr(SPRN_DSCR);
 }
 
 inline void set_dscr_usr(unsigned long val)
 {
-	asm volatile("mtspr %1,%0" : : "r" (val), "i" (SPRN_DSCR));
+	mtspr(SPRN_DSCR, val);
 }
 
 /* Default DSCR access */
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c b/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
index a0635a3819aa..1345e9b9af0f 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
@@ -23,6 +23,7 @@
 #include <sys/syscall.h>
 #include <linux/limits.h>
 #include "ptrace.h"
+#include "reg.h"
 
 #define SPRN_PVR	0x11F
 #define PVR_8xx		0x00500000
@@ -620,10 +621,7 @@ static int ptrace_hwbreak(void)
 
 int main(int argc, char **argv, char **envp)
 {
-	int pvr = 0;
-	asm __volatile__ ("mfspr %0,%1" : "=r"(pvr) : "i"(SPRN_PVR));
-	if (pvr == PVR_8xx)
-		is_8xx = true;
+	is_8xx = mfspr(SPRN_PVR) == PVR_8xx;
 
 	return test_harness(ptrace_hwbreak, "ptrace-hwbreak");
 }
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace.h b/tools/testing/selftests/powerpc/ptrace/ptrace.h
index 4e0233c0f2b3..04788e5fc504 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace.h
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace.h
@@ -745,10 +745,7 @@ int show_tm_spr(pid_t child, struct tm_spr_regs *out)
 /* Analyse TEXASR after TM failure */
 inline unsigned long get_tfiar(void)
 {
-	unsigned long ret;
-
-	asm volatile("mfspr %0,%1" : "=r" (ret) : "i" (SPRN_TFIAR));
-	return ret;
+	return mfspr(SPRN_TFIAR);
 }
 
 void analyse_texasr(unsigned long texasr)
diff --git a/tools/testing/selftests/powerpc/security/flush_utils.c b/tools/testing/selftests/powerpc/security/flush_utils.c
index 4d95965cb751..9c5c00e04f63 100644
--- a/tools/testing/selftests/powerpc/security/flush_utils.c
+++ b/tools/testing/selftests/powerpc/security/flush_utils.c
@@ -14,6 +14,7 @@
 #include <string.h>
 #include <stdio.h>
 #include <sys/utsname.h>
+#include "reg.h"
 #include "utils.h"
 #include "flush_utils.h"
 
@@ -79,5 +80,5 @@ void set_dscr(unsigned long val)
 		init = 1;
 	}
 
-	asm volatile("mtspr %1,%0" : : "r" (val), "i" (SPRN_DSCR));
+	mtspr(SPRN_DSCR, val);
 }
-- 
2.38.1

[PATCH v3 2/7] selftests/powerpc: Add ptrace setup_core_pattern() null-terminator

[Benjamin Gray]

- malloc() does not zero the buffer,
- fread() does not null-terminate it's output,
- `cat /proc/sys/kernel/core_pattern | hexdump -C` shows the file is
  not inherently null-terminated

So using string operations on the buffer is risky. Explicitly add a null
character to the end to make it safer.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 tools/testing/selftests/powerpc/ptrace/core-pkey.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/powerpc/ptrace/core-pkey.c b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
index bbc05ffc5860..5c82ed9e7c65 100644
--- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
@@ -383,7 +383,7 @@ static int setup_core_pattern(char **core_pattern_, bool *changed_)
 		goto out;
 	}
 
-	ret = fread(core_pattern, 1, PATH_MAX, f);
+	ret = fread(core_pattern, 1, PATH_MAX - 1, f);
 	fclose(f);
 	if (!ret) {
 		perror("Error reading core_pattern file");
@@ -391,6 +391,8 @@ static int setup_core_pattern(char **core_pattern_, bool *changed_)
 		goto out;
 	}
 
+	core_pattern[ret] = '\0';
+
 	/* Check whether we can predict the name of the core file. */
 	if (!strcmp(core_pattern, "core") || !strcmp(core_pattern, "core.%p"))
 		*changed_ = false;
-- 
2.38.1

[PATCH v3 3/7] selftests/powerpc: Add generic read/write file util

[Benjamin Gray]

File read/write is reimplemented in about 5 different ways in the
various PowerPC selftests. This indicates it should be a common util.

Add a common read_file / write_file implementation and convert users
to it where (easily) possible.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 tools/testing/selftests/powerpc/dscr/dscr.h   |  36 ++----
 .../selftests/powerpc/dscr/dscr_sysfs_test.c  |  19 +--
 .../testing/selftests/powerpc/include/utils.h |   2 +
 .../selftests/powerpc/nx-gzip/gzfht_test.c    |  49 +++-----
 tools/testing/selftests/powerpc/pmu/lib.c     |  27 +----
 .../selftests/powerpc/ptrace/core-pkey.c      |  30 ++---
 tools/testing/selftests/powerpc/utils.c       | 108 ++++++++++--------
 7 files changed, 107 insertions(+), 164 deletions(-)

diff --git a/tools/testing/selftests/powerpc/dscr/dscr.h b/tools/testing/selftests/powerpc/dscr/dscr.h
index b703714e7d98..9a69d473ffdf 100644
--- a/tools/testing/selftests/powerpc/dscr/dscr.h
+++ b/tools/testing/selftests/powerpc/dscr/dscr.h
@@ -64,48 +64,30 @@ inline void set_dscr_usr(unsigned long val)
 /* Default DSCR access */
 unsigned long get_default_dscr(void)
 {
-	int fd = -1, ret;
-	char buf[16];
+	int err;
+	char buf[16] = {0};
 	unsigned long val;
 
-	if (fd == -1) {
-		fd = open(DSCR_DEFAULT, O_RDONLY);
-		if (fd == -1) {
-			perror("open() failed");
-			exit(1);
-		}
-	}
-	memset(buf, 0, sizeof(buf));
-	lseek(fd, 0, SEEK_SET);
-	ret = read(fd, buf, sizeof(buf));
-	if (ret == -1) {
-		perror("read() failed");
+	if ((err = read_file(DSCR_DEFAULT, buf, sizeof(buf) - 1, NULL))) {
+		fprintf(stderr, "get_default_dscr() read failed: %s\n", strerror(err));
 		exit(1);
 	}
+
 	sscanf(buf, "%lx", &val);
-	close(fd);
 	return val;
 }
 
 void set_default_dscr(unsigned long val)
 {
-	int fd = -1, ret;
+	int err;
 	char buf[16];
 
-	if (fd == -1) {
-		fd = open(DSCR_DEFAULT, O_RDWR);
-		if (fd == -1) {
-			perror("open() failed");
-			exit(1);
-		}
-	}
 	sprintf(buf, "%lx\n", val);
-	ret = write(fd, buf, strlen(buf));
-	if (ret == -1) {
-		perror("write() failed");
+
+	if ((err = write_file(DSCR_DEFAULT, buf, strlen(buf)))) {
+		fprintf(stderr, "set_default_dscr() write failed: %s\n", strerror(err));
 		exit(1);
 	}
-	close(fd);
 }
 
 double uniform_deviate(int seed)
diff --git a/tools/testing/selftests/powerpc/dscr/dscr_sysfs_test.c b/tools/testing/selftests/powerpc/dscr/dscr_sysfs_test.c
index fbbdffdb2e5d..310946262a24 100644
--- a/tools/testing/selftests/powerpc/dscr/dscr_sysfs_test.c
+++ b/tools/testing/selftests/powerpc/dscr/dscr_sysfs_test.c
@@ -12,23 +12,12 @@
 
 static int check_cpu_dscr_default(char *file, unsigned long val)
 {
-	char buf[10];
-	int fd, rc;
+	char buf[10] = {0};
+	int rc;
 
-	fd = open(file, O_RDWR);
-	if (fd == -1) {
-		perror("open() failed");
-		return 1;
-	}
-
-	rc = read(fd, buf, sizeof(buf));
-	if (rc == -1) {
-		perror("read() failed");
-		return 1;
-	}
-	close(fd);
+	if ((rc = read_file(file, buf, sizeof(buf) - 1, NULL)))
+		return rc;
 
-	buf[rc] = '\0';
 	if (strtol(buf, NULL, 16) != val) {
 		printf("DSCR match failed: %ld (system) %ld (cpu)\n",
 					val, strtol(buf, NULL, 16));
diff --git a/tools/testing/selftests/powerpc/include/utils.h b/tools/testing/selftests/powerpc/include/utils.h
index e222a5858450..70885e5814a8 100644
--- a/tools/testing/selftests/powerpc/include/utils.h
+++ b/tools/testing/selftests/powerpc/include/utils.h
@@ -33,6 +33,8 @@ void *get_auxv_entry(int type);
 
 int pick_online_cpu(void);
 
+int read_file(const char *path, char *buf, size_t count, size_t *len);
+int write_file(const char *path, const char *buf, size_t count);
 int read_debugfs_file(char *debugfs_file, int *result);
 int write_debugfs_file(char *debugfs_file, int result);
 int read_sysfs_file(char *debugfs_file, char *result, size_t result_size);
diff --git a/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c b/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
index 095195a25687..a6a226e1b8ba 100644
--- a/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
+++ b/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
@@ -146,49 +146,36 @@ int gzip_header_blank(char *buf)
 /* Caller must free the allocated buffer return nonzero on error. */
 int read_alloc_input_file(char *fname, char **buf, size_t *bufsize)
 {
+	int err;
 	struct stat statbuf;
-	FILE *fp;
 	char *p;
 	size_t num_bytes;
 
 	if (stat(fname, &statbuf)) {
 		perror(fname);
-		return(-1);
-	}
-	fp = fopen(fname, "r");
-	if (fp == NULL) {
-		perror(fname);
-		return(-1);
+		return -1;
 	}
+
 	assert(NULL != (p = (char *) malloc(statbuf.st_size)));
-	num_bytes = fread(p, 1, statbuf.st_size, fp);
-	if (ferror(fp) || (num_bytes != statbuf.st_size)) {
-		perror(fname);
-		return(-1);
+
+	if ((err = read_file(fname, p, statbuf.st_size, &num_bytes))) {
+		fprintf(stderr, "Failed to read file: %s\n", strerror(err));
+		goto fail;
 	}
+
+	if (num_bytes != statbuf.st_size) {
+		fprintf(stderr, "Actual bytes != expected bytes\n");
+		err = -1;
+		goto fail;
+	}
+
 	*buf = p;
 	*bufsize = num_bytes;
 	return 0;
-}
 
-/* Returns nonzero on error */
-int write_output_file(char *fname, char *buf, size_t bufsize)
-{
-	FILE *fp;
-	size_t num_bytes;
-
-	fp = fopen(fname, "w");
-	if (fp == NULL) {
-		perror(fname);
-		return(-1);
-	}
-	num_bytes = fwrite(buf, 1, bufsize, fp);
-	if (ferror(fp) || (num_bytes != bufsize)) {
-		perror(fname);
-		return(-1);
-	}
-	fclose(fp);
-	return 0;
+fail:
+	free(p);
+	return err;
 }
 
 /*
@@ -399,7 +386,7 @@ int compress_file(int argc, char **argv, void *handle)
 	assert(FNAME_MAX > (strlen(argv[1]) + strlen(FEXT)));
 	strcpy(outname, argv[1]);
 	strcat(outname, FEXT);
-	if (write_output_file(outname, outbuf, dsttotlen)) {
+	if (write_file(outname, outbuf, dsttotlen)) {
 		fprintf(stderr, "write error: %s\n", outname);
 		exit(-1);
 	}
diff --git a/tools/testing/selftests/powerpc/pmu/lib.c b/tools/testing/selftests/powerpc/pmu/lib.c
index 88690b97b7b9..e8960e7a1271 100644
--- a/tools/testing/selftests/powerpc/pmu/lib.c
+++ b/tools/testing/selftests/powerpc/pmu/lib.c
@@ -190,38 +190,21 @@ int parse_proc_maps(void)
 
 bool require_paranoia_below(int level)
 {
+	int err;
 	long current;
 	char *end, buf[16];
-	FILE *f;
-	bool rc;
-
-	rc = false;
-
-	f = fopen(PARANOID_PATH, "r");
-	if (!f) {
-		perror("fopen");
-		goto out;
-	}
 
-	if (!fgets(buf, sizeof(buf), f)) {
+	if ((err = read_file(PARANOID_PATH, buf, sizeof(buf), NULL))) {
 		printf("Couldn't read " PARANOID_PATH "?\n");
-		goto out_close;
+		return false;
 	}
 
 	current = strtol(buf, &end, 10);
 
 	if (end == buf) {
 		printf("Couldn't parse " PARANOID_PATH "?\n");
-		goto out_close;
+		return false;
 	}
 
-	if (current >= level)
-		goto out_close;
-
-	rc = true;
-out_close:
-	fclose(f);
-out:
-	return rc;
+	return current < level;
 }
-
diff --git a/tools/testing/selftests/powerpc/ptrace/core-pkey.c b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
index 5c82ed9e7c65..46e0695ed8b0 100644
--- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
@@ -348,16 +348,11 @@ static int parent(struct shared_info *info, pid_t pid)
 
 static int write_core_pattern(const char *core_pattern)
 {
-	size_t len = strlen(core_pattern), ret;
-	FILE *f;
+	int err;
 
-	f = fopen(core_pattern_file, "w");
-	SKIP_IF_MSG(!f, "Try with root privileges");
-
-	ret = fwrite(core_pattern, 1, len, f);
-	fclose(f);
-	if (ret != len) {
-		perror("Error writing to core_pattern file");
+	if ((err = write_file(core_pattern_file, core_pattern, strlen(core_pattern)))) {
+		SKIP_IF_MSG(err == -EPERM, "Try with root privileges");
+		fprintf(stderr, "Error writing core_pattern file: %s\n", strerror(err));
 		return TEST_FAIL;
 	}
 
@@ -366,8 +361,8 @@ static int write_core_pattern(const char *core_pattern)
 
 static int setup_core_pattern(char **core_pattern_, bool *changed_)
 {
-	FILE *f;
 	char *core_pattern;
+	size_t len;
 	int ret;
 
 	core_pattern = malloc(PATH_MAX);
@@ -376,22 +371,13 @@ static int setup_core_pattern(char **core_pattern_, bool *changed_)
 		return TEST_FAIL;
 	}
 
-	f = fopen(core_pattern_file, "r");
-	if (!f) {
-		perror("Error opening core_pattern file");
-		ret = TEST_FAIL;
-		goto out;
-	}
-
-	ret = fread(core_pattern, 1, PATH_MAX - 1, f);
-	fclose(f);
-	if (!ret) {
-		perror("Error reading core_pattern file");
+	if ((ret = read_file(core_pattern_file, core_pattern, PATH_MAX - 1, &len))) {
+		fprintf(stderr, "Error reading core_pattern file: %s\n", strerror(ret));
 		ret = TEST_FAIL;
 		goto out;
 	}
 
-	core_pattern[ret] = '\0';
+	core_pattern[len] = '\0';
 
 	/* Check whether we can predict the name of the core file. */
 	if (!strcmp(core_pattern, "core") || !strcmp(core_pattern, "core.%p"))
diff --git a/tools/testing/selftests/powerpc/utils.c b/tools/testing/selftests/powerpc/utils.c
index 1f36ee1a909a..861b1f7aa95f 100644
--- a/tools/testing/selftests/powerpc/utils.c
+++ b/tools/testing/selftests/powerpc/utils.c
@@ -26,34 +26,73 @@
 
 static char auxv[4096];
 
-int read_auxv(char *buf, ssize_t buf_size)
+int read_file(const char *path, char *buf, size_t count, size_t *len)
 {
-	ssize_t num;
-	int rc, fd;
+	ssize_t rc;
+	int fd;
+	int err;
+	char eof;
+
+	if ((fd = open(path, O_RDONLY)) < 0)
+		return errno;
 
-	fd = open("/proc/self/auxv", O_RDONLY);
-	if (fd == -1) {
-		perror("open");
-		return -errno;
+	if ((rc = read(fd, buf, count)) < 0) {
+		err = errno;
+		goto out;
 	}
 
-	num = read(fd, buf, buf_size);
-	if (num < 0) {
-		perror("read");
-		rc = -EIO;
+	if (len)
+		*len = rc;
+
+	/* Overflow if there are still more bytes after filling the buffer */
+	if (rc == count && (rc = read(fd, &eof, 1)) != 0) {
+		err = EOVERFLOW;
 		goto out;
 	}
 
-	if (num > buf_size) {
-		printf("overflowed auxv buffer\n");
-		rc = -EOVERFLOW;
+	err = 0;
+
+out:
+	close(fd);
+	return err;
+}
+
+int write_file(const char *path, const char *buf, size_t count)
+{
+	int fd;
+	int err;
+	ssize_t rc;
+
+	if ((fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0644)) < 0)
+		return errno;
+
+	if ((rc = write(fd, buf, count)) < 0) {
+		err = errno;
 		goto out;
 	}
 
-	rc = 0;
+	if (rc != count) {
+		err = EOVERFLOW;
+		goto out;
+	}
+
+	err = 0;
+
 out:
 	close(fd);
-	return rc;
+	return err;
+}
+
+int read_auxv(char *buf, ssize_t buf_size)
+{
+	int err;
+
+	if ((err = read_file("/proc/self/auxv", buf, buf_size, NULL))) {
+		fprintf(stderr, "Error reading auxv: %s\n", strerror(err));
+		return err;
+	}
+
+	return 0;
 }
 
 void *find_auxv_entry(int type, char *auxv)
@@ -142,65 +181,40 @@ bool is_ppc64le(void)
 int read_sysfs_file(char *fpath, char *result, size_t result_size)
 {
 	char path[PATH_MAX] = "/sys/";
-	int rc = -1, fd;
 
 	strncat(path, fpath, PATH_MAX - strlen(path) - 1);
 
-	if ((fd = open(path, O_RDONLY)) < 0)
-		return rc;
-
-	rc = read(fd, result, result_size);
-
-	close(fd);
-
-	if (rc < 0)
-		return rc;
-
-	return 0;
+	return read_file(path, result, result_size, NULL);
 }
 
 int read_debugfs_file(char *debugfs_file, int *result)
 {
-	int rc = -1, fd;
+	int err;
 	char path[PATH_MAX];
-	char value[16];
+	char value[16] = {0};
 
 	strcpy(path, "/sys/kernel/debug/");
 	strncat(path, debugfs_file, PATH_MAX - strlen(path) - 1);
 
-	if ((fd = open(path, O_RDONLY)) < 0)
-		return rc;
-
-	if ((rc = read(fd, value, sizeof(value))) < 0)
-		return rc;
+	if ((err = read_file(path, value, sizeof(value) - 1, NULL)))
+		return err;
 
-	value[15] = 0;
 	*result = atoi(value);
-	close(fd);
 
 	return 0;
 }
 
 int write_debugfs_file(char *debugfs_file, int result)
 {
-	int rc = -1, fd;
 	char path[PATH_MAX];
 	char value[16];
 
 	strcpy(path, "/sys/kernel/debug/");
 	strncat(path, debugfs_file, PATH_MAX - strlen(path) - 1);
 
-	if ((fd = open(path, O_WRONLY)) < 0)
-		return rc;
-
 	snprintf(value, 16, "%d", result);
 
-	if ((rc = write(fd, value, strlen(value))) < 0)
-		return rc;
-
-	close(fd);
-
-	return 0;
+	return write_file(path, value, strlen(value));
 }
 
 static long perf_event_open(struct perf_event_attr *hw_event, pid_t pid,
-- 
2.38.1

[PATCH v3 4/7] selftests/powerpc: Add read/write debugfs file, int

[Benjamin Gray]

Debugfs files are not always integers, so make *_file return/write a
byte buffer, and *_int deal with int values specifically. This increases
consistency with the other file read/write helpers.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 .../testing/selftests/powerpc/include/utils.h |  6 ++--
 .../selftests/powerpc/security/entry_flush.c  | 12 +++----
 .../selftests/powerpc/security/rfi_flush.c    | 12 +++----
 .../powerpc/security/uaccess_flush.c          | 18 +++++-----
 tools/testing/selftests/powerpc/utils.c       | 34 ++++++++++++-------
 5 files changed, 47 insertions(+), 35 deletions(-)

diff --git a/tools/testing/selftests/powerpc/include/utils.h b/tools/testing/selftests/powerpc/include/utils.h
index 70885e5814a8..de5e3790f397 100644
--- a/tools/testing/selftests/powerpc/include/utils.h
+++ b/tools/testing/selftests/powerpc/include/utils.h
@@ -35,8 +35,10 @@ int pick_online_cpu(void);
 
 int read_file(const char *path, char *buf, size_t count, size_t *len);
 int write_file(const char *path, const char *buf, size_t count);
-int read_debugfs_file(char *debugfs_file, int *result);
-int write_debugfs_file(char *debugfs_file, int result);
+int read_debugfs_file(const char *debugfs_file, char *buf, size_t count);
+int write_debugfs_file(const char *debugfs_file, const char *buf, size_t count);
+int read_debugfs_int(const char *debugfs_file, int *result);
+int write_debugfs_int(const char *debugfs_file, int result);
 int read_sysfs_file(char *debugfs_file, char *result, size_t result_size);
 int perf_event_open_counter(unsigned int type,
 			    unsigned long config, int group_fd);
diff --git a/tools/testing/selftests/powerpc/security/entry_flush.c b/tools/testing/selftests/powerpc/security/entry_flush.c
index 68ce377b205e..e01c573deadd 100644
--- a/tools/testing/selftests/powerpc/security/entry_flush.c
+++ b/tools/testing/selftests/powerpc/security/entry_flush.c
@@ -34,18 +34,18 @@ int entry_flush_test(void)
 	// The PMU event we use only works on Power7 or later
 	SKIP_IF(!have_hwcap(PPC_FEATURE_ARCH_2_06));
 
-	if (read_debugfs_file("powerpc/rfi_flush", &rfi_flush_orig) < 0) {
+	if (read_debugfs_int("powerpc/rfi_flush", &rfi_flush_orig) < 0) {
 		perror("Unable to read powerpc/rfi_flush debugfs file");
 		SKIP_IF(1);
 	}
 
-	if (read_debugfs_file("powerpc/entry_flush", &entry_flush_orig) < 0) {
+	if (read_debugfs_int("powerpc/entry_flush", &entry_flush_orig) < 0) {
 		perror("Unable to read powerpc/entry_flush debugfs file");
 		SKIP_IF(1);
 	}
 
 	if (rfi_flush_orig != 0) {
-		if (write_debugfs_file("powerpc/rfi_flush", 0) < 0) {
+		if (write_debugfs_int("powerpc/rfi_flush", 0) < 0) {
 			perror("error writing to powerpc/rfi_flush debugfs file");
 			FAIL_IF(1);
 		}
@@ -105,7 +105,7 @@ int entry_flush_test(void)
 
 	if (entry_flush == entry_flush_orig) {
 		entry_flush = !entry_flush_orig;
-		if (write_debugfs_file("powerpc/entry_flush", entry_flush) < 0) {
+		if (write_debugfs_int("powerpc/entry_flush", entry_flush) < 0) {
 			perror("error writing to powerpc/entry_flush debugfs file");
 			return 1;
 		}
@@ -120,12 +120,12 @@ int entry_flush_test(void)
 
 	set_dscr(0);
 
-	if (write_debugfs_file("powerpc/rfi_flush", rfi_flush_orig) < 0) {
+	if (write_debugfs_int("powerpc/rfi_flush", rfi_flush_orig) < 0) {
 		perror("unable to restore original value of powerpc/rfi_flush debugfs file");
 		return 1;
 	}
 
-	if (write_debugfs_file("powerpc/entry_flush", entry_flush_orig) < 0) {
+	if (write_debugfs_int("powerpc/entry_flush", entry_flush_orig) < 0) {
 		perror("unable to restore original value of powerpc/entry_flush debugfs file");
 		return 1;
 	}
diff --git a/tools/testing/selftests/powerpc/security/rfi_flush.c b/tools/testing/selftests/powerpc/security/rfi_flush.c
index f73484a6470f..6bedc86443a6 100644
--- a/tools/testing/selftests/powerpc/security/rfi_flush.c
+++ b/tools/testing/selftests/powerpc/security/rfi_flush.c
@@ -34,18 +34,18 @@ int rfi_flush_test(void)
 	// The PMU event we use only works on Power7 or later
 	SKIP_IF(!have_hwcap(PPC_FEATURE_ARCH_2_06));
 
-	if (read_debugfs_file("powerpc/rfi_flush", &rfi_flush_orig) < 0) {
+	if (read_debugfs_int("powerpc/rfi_flush", &rfi_flush_orig) < 0) {
 		perror("Unable to read powerpc/rfi_flush debugfs file");
 		SKIP_IF(1);
 	}
 
-	if (read_debugfs_file("powerpc/entry_flush", &entry_flush_orig) < 0) {
+	if (read_debugfs_int("powerpc/entry_flush", &entry_flush_orig) < 0) {
 		have_entry_flush = 0;
 	} else {
 		have_entry_flush = 1;
 
 		if (entry_flush_orig != 0) {
-			if (write_debugfs_file("powerpc/entry_flush", 0) < 0) {
+			if (write_debugfs_int("powerpc/entry_flush", 0) < 0) {
 				perror("error writing to powerpc/entry_flush debugfs file");
 				return 1;
 			}
@@ -105,7 +105,7 @@ int rfi_flush_test(void)
 
 	if (rfi_flush == rfi_flush_orig) {
 		rfi_flush = !rfi_flush_orig;
-		if (write_debugfs_file("powerpc/rfi_flush", rfi_flush) < 0) {
+		if (write_debugfs_int("powerpc/rfi_flush", rfi_flush) < 0) {
 			perror("error writing to powerpc/rfi_flush debugfs file");
 			return 1;
 		}
@@ -120,13 +120,13 @@ int rfi_flush_test(void)
 
 	set_dscr(0);
 
-	if (write_debugfs_file("powerpc/rfi_flush", rfi_flush_orig) < 0) {
+	if (write_debugfs_int("powerpc/rfi_flush", rfi_flush_orig) < 0) {
 		perror("unable to restore original value of powerpc/rfi_flush debugfs file");
 		return 1;
 	}
 
 	if (have_entry_flush) {
-		if (write_debugfs_file("powerpc/entry_flush", entry_flush_orig) < 0) {
+		if (write_debugfs_int("powerpc/entry_flush", entry_flush_orig) < 0) {
 			perror("unable to restore original value of powerpc/entry_flush "
 			       "debugfs file");
 			return 1;
diff --git a/tools/testing/selftests/powerpc/security/uaccess_flush.c b/tools/testing/selftests/powerpc/security/uaccess_flush.c
index cf80f960e38a..fcf23ea9b183 100644
--- a/tools/testing/selftests/powerpc/security/uaccess_flush.c
+++ b/tools/testing/selftests/powerpc/security/uaccess_flush.c
@@ -36,30 +36,30 @@ int uaccess_flush_test(void)
 	// The PMU event we use only works on Power7 or later
 	SKIP_IF(!have_hwcap(PPC_FEATURE_ARCH_2_06));
 
-	if (read_debugfs_file("powerpc/rfi_flush", &rfi_flush_orig) < 0) {
+	if (read_debugfs_int("powerpc/rfi_flush", &rfi_flush_orig) < 0) {
 		perror("Unable to read powerpc/rfi_flush debugfs file");
 		SKIP_IF(1);
 	}
 
-	if (read_debugfs_file("powerpc/entry_flush", &entry_flush_orig) < 0) {
+	if (read_debugfs_int("powerpc/entry_flush", &entry_flush_orig) < 0) {
 		perror("Unable to read powerpc/entry_flush debugfs file");
 		SKIP_IF(1);
 	}
 
-	if (read_debugfs_file("powerpc/uaccess_flush", &uaccess_flush_orig) < 0) {
+	if (read_debugfs_int("powerpc/uaccess_flush", &uaccess_flush_orig) < 0) {
 		perror("Unable to read powerpc/entry_flush debugfs file");
 		SKIP_IF(1);
 	}
 
 	if (rfi_flush_orig != 0) {
-		if (write_debugfs_file("powerpc/rfi_flush", 0) < 0) {
+		if (write_debugfs_int("powerpc/rfi_flush", 0) < 0) {
 			perror("error writing to powerpc/rfi_flush debugfs file");
 			FAIL_IF(1);
 		}
 	}
 
 	if (entry_flush_orig != 0) {
-		if (write_debugfs_file("powerpc/entry_flush", 0) < 0) {
+		if (write_debugfs_int("powerpc/entry_flush", 0) < 0) {
 			perror("error writing to powerpc/entry_flush debugfs file");
 			FAIL_IF(1);
 		}
@@ -119,7 +119,7 @@ int uaccess_flush_test(void)
 
 	if (uaccess_flush == uaccess_flush_orig) {
 		uaccess_flush = !uaccess_flush_orig;
-		if (write_debugfs_file("powerpc/uaccess_flush", uaccess_flush) < 0) {
+		if (write_debugfs_int("powerpc/uaccess_flush", uaccess_flush) < 0) {
 			perror("error writing to powerpc/uaccess_flush debugfs file");
 			return 1;
 		}
@@ -134,17 +134,17 @@ int uaccess_flush_test(void)
 
 	set_dscr(0);
 
-	if (write_debugfs_file("powerpc/rfi_flush", rfi_flush_orig) < 0) {
+	if (write_debugfs_int("powerpc/rfi_flush", rfi_flush_orig) < 0) {
 		perror("unable to restore original value of powerpc/rfi_flush debugfs file");
 		return 1;
 	}
 
-	if (write_debugfs_file("powerpc/entry_flush", entry_flush_orig) < 0) {
+	if (write_debugfs_int("powerpc/entry_flush", entry_flush_orig) < 0) {
 		perror("unable to restore original value of powerpc/entry_flush debugfs file");
 		return 1;
 	}
 
-	if (write_debugfs_file("powerpc/uaccess_flush", uaccess_flush_orig) < 0) {
+	if (write_debugfs_int("powerpc/uaccess_flush", uaccess_flush_orig) < 0) {
 		perror("unable to restore original value of powerpc/uaccess_flush debugfs file");
 		return 1;
 	}
diff --git a/tools/testing/selftests/powerpc/utils.c b/tools/testing/selftests/powerpc/utils.c
index 861b1f7aa95f..8593e67ce779 100644
--- a/tools/testing/selftests/powerpc/utils.c
+++ b/tools/testing/selftests/powerpc/utils.c
@@ -95,6 +95,24 @@ int read_auxv(char *buf, ssize_t buf_size)
 	return 0;
 }
 
+int read_debugfs_file(const char *subpath, char *buf, size_t count)
+{
+	char path[PATH_MAX] = "/sys/kernel/debug/";
+
+	strncat(path, subpath, sizeof(path) - strlen(path) - 1);
+
+	return read_file(path, buf, count, NULL);
+}
+
+int write_debugfs_file(const char *subpath, const char *buf, size_t count)
+{
+	char path[PATH_MAX] = "/sys/kernel/debug/";
+
+	strncat(path, subpath, sizeof(path) - strlen(path) - 1);
+
+	return write_file(path, buf, count);
+}
+
 void *find_auxv_entry(int type, char *auxv)
 {
 	ElfW(auxv_t) *p;
@@ -187,16 +205,12 @@ int read_sysfs_file(char *fpath, char *result, size_t result_size)
 	return read_file(path, result, result_size, NULL);
 }
 
-int read_debugfs_file(char *debugfs_file, int *result)
+int read_debugfs_int(const char *debugfs_file, int *result)
 {
 	int err;
-	char path[PATH_MAX];
 	char value[16] = {0};
 
-	strcpy(path, "/sys/kernel/debug/");
-	strncat(path, debugfs_file, PATH_MAX - strlen(path) - 1);
-
-	if ((err = read_file(path, value, sizeof(value) - 1, NULL)))
+	if ((err = read_debugfs_file(debugfs_file, value, sizeof(value) - 1)))
 		return err;
 
 	*result = atoi(value);
@@ -204,17 +218,13 @@ int read_debugfs_file(char *debugfs_file, int *result)
 	return 0;
 }
 
-int write_debugfs_file(char *debugfs_file, int result)
+int write_debugfs_int(const char *debugfs_file, int result)
 {
-	char path[PATH_MAX];
 	char value[16];
 
-	strcpy(path, "/sys/kernel/debug/");
-	strncat(path, debugfs_file, PATH_MAX - strlen(path) - 1);
-
 	snprintf(value, 16, "%d", result);
 
-	return write_file(path, value, strlen(value));
+	return write_debugfs_file(debugfs_file, value, strlen(value));
 }
 
 static long perf_event_open(struct perf_event_attr *hw_event, pid_t pid,
-- 
2.38.1

[PATCH v3 5/7] selftests/powerpc: Parse long/unsigned long value safely

[Benjamin Gray]

Often a file is expected to hold an integral value. Existing functions
will use a C stdlib function like atoi or strtol to parse the file.
These operations are error prone, with complicated error conditions
(atoi returns 0 if not a number, and is undefined behaviour if not in
range. strtol returns 0 if not a number, and LONG_MIN/MAX if not in
range + sets errno to ERANGE).

Add a dedicated parse function that accounts for these error conditions
so tests can safely parse numbers without undetected bad data. It's a
bit ugly to generate the functions through a macro, but it beats copying
the error check logic multiple times over.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 .../testing/selftests/powerpc/include/utils.h |  5 ++
 tools/testing/selftests/powerpc/pmu/lib.c     |  9 ++--
 tools/testing/selftests/powerpc/utils.c       | 53 +++++++++++++++++--
 3 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/powerpc/include/utils.h b/tools/testing/selftests/powerpc/include/utils.h
index de5e3790f397..b82e143a07c6 100644
--- a/tools/testing/selftests/powerpc/include/utils.h
+++ b/tools/testing/selftests/powerpc/include/utils.h
@@ -33,6 +33,11 @@ void *get_auxv_entry(int type);
 
 int pick_online_cpu(void);
 
+int parse_int(const char *buffer, size_t count, int *result, int base);
+int parse_long(const char *buffer, size_t count, long *result, int base);
+int parse_uint(const char *buffer, size_t count, unsigned int *result, int base);
+int parse_ulong(const char *buffer, size_t count, unsigned long *result, int base);
+
 int read_file(const char *path, char *buf, size_t count, size_t *len);
 int write_file(const char *path, const char *buf, size_t count);
 int read_debugfs_file(const char *debugfs_file, char *buf, size_t count);
diff --git a/tools/testing/selftests/powerpc/pmu/lib.c b/tools/testing/selftests/powerpc/pmu/lib.c
index e8960e7a1271..771658278f55 100644
--- a/tools/testing/selftests/powerpc/pmu/lib.c
+++ b/tools/testing/selftests/powerpc/pmu/lib.c
@@ -192,16 +192,15 @@ bool require_paranoia_below(int level)
 {
 	int err;
 	long current;
-	char *end, buf[16];
+	char buf[16] = {0};
+	char *end;
 
-	if ((err = read_file(PARANOID_PATH, buf, sizeof(buf), NULL))) {
+	if ((err = read_file(PARANOID_PATH, buf, sizeof(buf) - 1, NULL))) {
 		printf("Couldn't read " PARANOID_PATH "?\n");
 		return false;
 	}
 
-	current = strtol(buf, &end, 10);
-
-	if (end == buf) {
+	if ((err = parse_long(buf, sizeof(buf), &current, 10))) {
 		printf("Couldn't parse " PARANOID_PATH "?\n");
 		return false;
 	}
diff --git a/tools/testing/selftests/powerpc/utils.c b/tools/testing/selftests/powerpc/utils.c
index 8593e67ce779..c82539fd44f1 100644
--- a/tools/testing/selftests/powerpc/utils.c
+++ b/tools/testing/selftests/powerpc/utils.c
@@ -8,6 +8,8 @@
 #include <elf.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <inttypes.h>
+#include <limits.h>
 #include <link.h>
 #include <sched.h>
 #include <stdio.h>
@@ -113,6 +115,53 @@ int write_debugfs_file(const char *subpath, const char *buf, size_t count)
 	return write_file(path, buf, count);
 }
 
+#define TYPE_MIN(x)				\
+	_Generic((x),				\
+		int:		INT_MIN,	\
+		long:		LONG_MIN,	\
+		unsigned int:	0,		\
+		unsigned long:	0)
+
+#define TYPE_MAX(x)				\
+	_Generic((x),				\
+		int:		INT_MAX,	\
+		long:		LONG_MAX,	\
+		unsigned int:	INT_MAX,	\
+		unsigned long:	LONG_MAX)
+
+#define define_parse_number(fn, type, super_type)				\
+	int fn(const char *buffer, size_t count, type *result, int base)	\
+	{									\
+		char *end;							\
+		super_type parsed;						\
+										\
+		errno = 0;							\
+		parsed = _Generic(parsed,					\
+				  intmax_t:	strtoimax,			\
+				  uintmax_t:	strtoumax)(buffer, &end, base);	\
+										\
+		if (errno == ERANGE ||						\
+		    parsed < TYPE_MIN(*result) || parsed > TYPE_MAX(*result))	\
+			return ERANGE;						\
+										\
+		/* Require at least one digit */				\
+		if (end == buffer)						\
+			return EINVAL;						\
+										\
+		/* Require all remaining characters be whitespace-ish */	\
+		for (; end < buffer + count; end++)				\
+			if (!(*end == ' ' || *end == '\n' || *end == '\0'))	\
+				return EINVAL;					\
+										\
+		*result = parsed;						\
+		return 0;							\
+	}
+
+define_parse_number(parse_int, int, intmax_t);
+define_parse_number(parse_long, long, intmax_t);
+define_parse_number(parse_uint, unsigned int, uintmax_t);
+define_parse_number(parse_ulong, unsigned long, uintmax_t);
+
 void *find_auxv_entry(int type, char *auxv)
 {
 	ElfW(auxv_t) *p;
@@ -213,9 +262,7 @@ int read_debugfs_int(const char *debugfs_file, int *result)
 	if ((err = read_debugfs_file(debugfs_file, value, sizeof(value) - 1)))
 		return err;
 
-	*result = atoi(value);
-
-	return 0;
+	return parse_int(value, sizeof(value), result, 10);
 }
 
 int write_debugfs_int(const char *debugfs_file, int result)
-- 
2.38.1

[PATCH v3 6/7] selftests/powerpc: Add {read,write}_{long,ulong}

[Benjamin Gray]

Add helper functions to read and write (unsigned) long values directly
from/to files. One of the kernel interfaces uses hex strings, so we need
to allow passing a base too.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 tools/testing/selftests/powerpc/dscr/dscr.h   |  9 +--
 .../selftests/powerpc/dscr/dscr_sysfs_test.c  | 12 ++--
 .../testing/selftests/powerpc/include/utils.h |  4 ++
 tools/testing/selftests/powerpc/pmu/lib.c     | 11 +---
 tools/testing/selftests/powerpc/utils.c       | 62 +++++++++++++++++++
 5 files changed, 76 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/powerpc/dscr/dscr.h b/tools/testing/selftests/powerpc/dscr/dscr.h
index 9a69d473ffdf..b5166ddcf26a 100644
--- a/tools/testing/selftests/powerpc/dscr/dscr.h
+++ b/tools/testing/selftests/powerpc/dscr/dscr.h
@@ -65,26 +65,21 @@ inline void set_dscr_usr(unsigned long val)
 unsigned long get_default_dscr(void)
 {
 	int err;
-	char buf[16] = {0};
 	unsigned long val;
 
-	if ((err = read_file(DSCR_DEFAULT, buf, sizeof(buf) - 1, NULL))) {
+	if ((err = read_ulong(DSCR_DEFAULT, &val, 16))) {
 		fprintf(stderr, "get_default_dscr() read failed: %s\n", strerror(err));
 		exit(1);
 	}
 
-	sscanf(buf, "%lx", &val);
 	return val;
 }
 
 void set_default_dscr(unsigned long val)
 {
 	int err;
-	char buf[16];
 
-	sprintf(buf, "%lx\n", val);
-
-	if ((err = write_file(DSCR_DEFAULT, buf, strlen(buf)))) {
+	if ((err = write_ulong(DSCR_DEFAULT, val, 16))) {
 		fprintf(stderr, "set_default_dscr() write failed: %s\n", strerror(err));
 		exit(1);
 	}
diff --git a/tools/testing/selftests/powerpc/dscr/dscr_sysfs_test.c b/tools/testing/selftests/powerpc/dscr/dscr_sysfs_test.c
index 310946262a24..3ac176888feb 100644
--- a/tools/testing/selftests/powerpc/dscr/dscr_sysfs_test.c
+++ b/tools/testing/selftests/powerpc/dscr/dscr_sysfs_test.c
@@ -12,15 +12,15 @@
 
 static int check_cpu_dscr_default(char *file, unsigned long val)
 {
-	char buf[10] = {0};
-	int rc;
+	unsigned long cpu_dscr;
+	int err;
 
-	if ((rc = read_file(file, buf, sizeof(buf) - 1, NULL)))
-		return rc;
+	if ((err = read_ulong(file, &cpu_dscr, 16)))
+		return err;
 
-	if (strtol(buf, NULL, 16) != val) {
+	if (cpu_dscr != val) {
 		printf("DSCR match failed: %ld (system) %ld (cpu)\n",
-					val, strtol(buf, NULL, 16));
+					val, cpu_dscr);
 		return 1;
 	}
 	return 0;
diff --git a/tools/testing/selftests/powerpc/include/utils.h b/tools/testing/selftests/powerpc/include/utils.h
index b82e143a07c6..044b0236df38 100644
--- a/tools/testing/selftests/powerpc/include/utils.h
+++ b/tools/testing/selftests/powerpc/include/utils.h
@@ -40,6 +40,10 @@ int parse_ulong(const char *buffer, size_t count, unsigned long *result, int bas
 
 int read_file(const char *path, char *buf, size_t count, size_t *len);
 int write_file(const char *path, const char *buf, size_t count);
+int read_long(const char *path, long *result, int base);
+int write_long(const char *path, long result, int base);
+int read_ulong(const char *path, unsigned long *result, int base);
+int write_ulong(const char *path, unsigned long result, int base);
 int read_debugfs_file(const char *debugfs_file, char *buf, size_t count);
 int write_debugfs_file(const char *debugfs_file, const char *buf, size_t count);
 int read_debugfs_int(const char *debugfs_file, int *result);
diff --git a/tools/testing/selftests/powerpc/pmu/lib.c b/tools/testing/selftests/powerpc/pmu/lib.c
index 771658278f55..55481c5b6995 100644
--- a/tools/testing/selftests/powerpc/pmu/lib.c
+++ b/tools/testing/selftests/powerpc/pmu/lib.c
@@ -192,16 +192,9 @@ bool require_paranoia_below(int level)
 {
 	int err;
 	long current;
-	char buf[16] = {0};
-	char *end;
 
-	if ((err = read_file(PARANOID_PATH, buf, sizeof(buf) - 1, NULL))) {
-		printf("Couldn't read " PARANOID_PATH "?\n");
-		return false;
-	}
-
-	if ((err = parse_long(buf, sizeof(buf), &current, 10))) {
-		printf("Couldn't parse " PARANOID_PATH "?\n");
+	if ((err = read_long(PARANOID_PATH, &current, 10))) {
+		fprintf(stderr, "Couldn't read " PARANOID_PATH ": %s\n", strerror(err));
 		return false;
 	}
 
diff --git a/tools/testing/selftests/powerpc/utils.c b/tools/testing/selftests/powerpc/utils.c
index c82539fd44f1..b2906dd71cf5 100644
--- a/tools/testing/selftests/powerpc/utils.c
+++ b/tools/testing/selftests/powerpc/utils.c
@@ -162,6 +162,68 @@ define_parse_number(parse_long, long, intmax_t);
 define_parse_number(parse_uint, unsigned int, uintmax_t);
 define_parse_number(parse_ulong, unsigned long, uintmax_t);
 
+int read_long(const char *path, long *result, int base)
+{
+	int err;
+	char buffer[32] = {0};
+
+	if ((err = read_file(path, buffer, sizeof(buffer) - 1, NULL)))
+		return err;
+
+	return parse_long(buffer, sizeof(buffer), result, base);
+}
+
+int read_ulong(const char *path, unsigned long *result, int base)
+{
+	int err;
+	char buffer[32] = {0};
+
+	if ((err = read_file(path, buffer, sizeof(buffer) - 1, NULL)))
+		return err;
+
+	return parse_ulong(buffer, sizeof(buffer), result, base);
+}
+
+int write_long(const char *path, long result, int base)
+{
+	int len;
+	char buffer[32];
+
+	/* Decimal only; we don't have a format specifier for signed hex values */
+	if (base != 10)
+		return EINVAL;
+
+	len = snprintf(buffer, sizeof(buffer), "%ld", result);
+	if (len < 0 || len >= sizeof(buffer))
+		return EOVERFLOW;
+
+	return write_file(path, buffer, len);
+}
+
+int write_ulong(const char *path, unsigned long result, int base)
+{
+	int len;
+	char buffer[32];
+	char *fmt;
+
+	switch (base) {
+	case 10:
+		fmt = "%lu";
+		break;
+	case 16:
+		fmt = "%lx";
+		break;
+	default:
+		return EINVAL;
+	}
+
+	len = snprintf(buffer, sizeof(buffer), fmt, result);
+	if (len < 0 || len >= sizeof(buffer))
+		return -1;
+
+	return write_file(path, buffer, len);
+}
+
 void *find_auxv_entry(int type, char *auxv)
 {
 	ElfW(auxv_t) *p;
-- 
2.38.1

[PATCH v3 7/7] selftests/powerpc: Add automatically allocating read_file

[Benjamin Gray]

A couple of tests roll their own auto-allocating file read logic.

Add a generic implementation and convert them to use it.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 .../testing/selftests/powerpc/include/utils.h |  1 +
 .../selftests/powerpc/nx-gzip/gzfht_test.c    | 37 +--------
 .../selftests/powerpc/syscalls/Makefile       |  2 +-
 .../selftests/powerpc/syscalls/rtas_filter.c  | 80 +++----------------
 tools/testing/selftests/powerpc/utils.c       | 63 +++++++++++++++
 5 files changed, 75 insertions(+), 108 deletions(-)

diff --git a/tools/testing/selftests/powerpc/include/utils.h b/tools/testing/selftests/powerpc/include/utils.h
index 044b0236df38..95f3a24a4569 100644
--- a/tools/testing/selftests/powerpc/include/utils.h
+++ b/tools/testing/selftests/powerpc/include/utils.h
@@ -40,6 +40,7 @@ int parse_ulong(const char *buffer, size_t count, unsigned long *result, int bas
 
 int read_file(const char *path, char *buf, size_t count, size_t *len);
 int write_file(const char *path, const char *buf, size_t count);
+int read_file_alloc(const char *path, char **buf, size_t *len);
 int read_long(const char *path, long *result, int base);
 int write_long(const char *path, long result, int base);
 int read_ulong(const char *path, unsigned long *result, int base);
diff --git a/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c b/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
index a6a226e1b8ba..4de079923ccb 100644
--- a/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
+++ b/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
@@ -143,41 +143,6 @@ int gzip_header_blank(char *buf)
 	return i;
 }
 
-/* Caller must free the allocated buffer return nonzero on error. */
-int read_alloc_input_file(char *fname, char **buf, size_t *bufsize)
-{
-	int err;
-	struct stat statbuf;
-	char *p;
-	size_t num_bytes;
-
-	if (stat(fname, &statbuf)) {
-		perror(fname);
-		return -1;
-	}
-
-	assert(NULL != (p = (char *) malloc(statbuf.st_size)));
-
-	if ((err = read_file(fname, p, statbuf.st_size, &num_bytes))) {
-		fprintf(stderr, "Failed to read file: %s\n", strerror(err));
-		goto fail;
-	}
-
-	if (num_bytes != statbuf.st_size) {
-		fprintf(stderr, "Actual bytes != expected bytes\n");
-		err = -1;
-		goto fail;
-	}
-
-	*buf = p;
-	*bufsize = num_bytes;
-	return 0;
-
-fail:
-	free(p);
-	return err;
-}
-
 /*
  * Z_SYNC_FLUSH as described in zlib.h.
  * Returns number of appended bytes
@@ -244,7 +209,7 @@ int compress_file(int argc, char **argv, void *handle)
 		fprintf(stderr, "usage: %s <fname>\n", argv[0]);
 		exit(-1);
 	}
-	if (read_alloc_input_file(argv[1], &inbuf, &inlen))
+	if (read_file_alloc(argv[1], &inbuf, &inlen))
 		exit(-1);
 	fprintf(stderr, "file %s read, %ld bytes\n", argv[1], inlen);
 
diff --git a/tools/testing/selftests/powerpc/syscalls/Makefile b/tools/testing/selftests/powerpc/syscalls/Makefile
index b63f8459c704..54ff5cfffc63 100644
--- a/tools/testing/selftests/powerpc/syscalls/Makefile
+++ b/tools/testing/selftests/powerpc/syscalls/Makefile
@@ -6,4 +6,4 @@ CFLAGS += -I../../../../../usr/include
 top_srcdir = ../../../../..
 include ../../lib.mk
 
-$(TEST_GEN_PROGS): ../harness.c
+$(TEST_GEN_PROGS): ../harness.c ../utils.c
diff --git a/tools/testing/selftests/powerpc/syscalls/rtas_filter.c b/tools/testing/selftests/powerpc/syscalls/rtas_filter.c
index 03b487f18d00..05f25f12556f 100644
--- a/tools/testing/selftests/powerpc/syscalls/rtas_filter.c
+++ b/tools/testing/selftests/powerpc/syscalls/rtas_filter.c
@@ -8,6 +8,7 @@
 #include <byteswap.h>
 #include <stdint.h>
 #include <inttypes.h>
+#include <linux/limits.h>
 #include <stdio.h>
 #include <string.h>
 #include <sys/syscall.h>
@@ -50,70 +51,16 @@ struct region {
 	struct region *next;
 };
 
-int read_entire_file(int fd, char **buf, size_t *len)
-{
-	size_t buf_size = 0;
-	size_t off = 0;
-	int rc;
-
-	*buf = NULL;
-	do {
-		buf_size += BLOCK_SIZE;
-		if (*buf == NULL)
-			*buf = malloc(buf_size);
-		else
-			*buf = realloc(*buf, buf_size);
-
-		if (*buf == NULL)
-			return -ENOMEM;
-
-		rc = read(fd, *buf + off, BLOCK_SIZE);
-		if (rc < 0)
-			return -EIO;
-
-		off += rc;
-	} while (rc == BLOCK_SIZE);
-
-	if (len)
-		*len = off;
-
-	return 0;
-}
-
-static int open_prop_file(const char *prop_path, const char *prop_name, int *fd)
-{
-	char *path;
-	int len;
-
-	/* allocate enough for two string, a slash and trailing NULL */
-	len = strlen(prop_path) + strlen(prop_name) + 1 + 1;
-	path = malloc(len);
-	if (path == NULL)
-		return -ENOMEM;
-
-	snprintf(path, len, "%s/%s", prop_path, prop_name);
-
-	*fd = open(path, O_RDONLY);
-	free(path);
-	if (*fd < 0)
-		return -errno;
-
-	return 0;
-}
-
 static int get_property(const char *prop_path, const char *prop_name,
 			char **prop_val, size_t *prop_len)
 {
-	int rc, fd;
-
-	rc = open_prop_file(prop_path, prop_name, &fd);
-	if (rc)
-		return rc;
+	char path[PATH_MAX];
 
-	rc = read_entire_file(fd, prop_val, prop_len);
-	close(fd);
+	int len = snprintf(path, sizeof(path), "%s/%s", prop_path, prop_name);
+	if (len < 0 || len >= sizeof(path))
+		return -ENOMEM;
 
-	return rc;
+	return read_file_alloc(path, prop_val, prop_len);
 }
 
 int rtas_token(const char *call_name)
@@ -138,22 +85,13 @@ int rtas_token(const char *call_name)
 static int read_kregion_bounds(struct region *kregion)
 {
 	char *buf;
-	int fd;
-	int rc;
+	int err;
 
-	fd = open("/proc/ppc64/rtas/rmo_buffer", O_RDONLY);
-	if (fd < 0) {
-		printf("Could not open rmo_buffer file\n");
+	if ((err = read_file_alloc("/proc/ppc64/rtas/rmo_buffer", &buf, NULL))) {
+		fprintf(stderr, "Error reading rmo_buffer: %s\n", strerror(err));
 		return RTAS_IO_ASSERT;
 	}
 
-	rc = read_entire_file(fd, &buf, NULL);
-	close(fd);
-	if (rc) {
-		free(buf);
-		return rc;
-	}
-
 	sscanf(buf, "%" SCNx64 " %x", &kregion->addr, &kregion->size);
 	free(buf);
 
diff --git a/tools/testing/selftests/powerpc/utils.c b/tools/testing/selftests/powerpc/utils.c
index b2906dd71cf5..52b18bf2020a 100644
--- a/tools/testing/selftests/powerpc/utils.c
+++ b/tools/testing/selftests/powerpc/utils.c
@@ -59,6 +59,69 @@ int read_file(const char *path, char *buf, size_t count, size_t *len)
 	return err;
 }
 
+int read_file_alloc(const char *path, char **buf, size_t *len)
+{
+	ssize_t rc;
+	char *buffer;
+	size_t read_offset;
+	size_t length;
+	int fd;
+	int err;
+
+
+	if ((fd = open(path, O_RDONLY)) < 0)
+		return -errno;
+
+	/*
+	 * We don't use stat & preallocate st_size because some non-files
+	 * report 0 file size. Instead just dynamically grow the buffer
+	 * as needed.
+	 */
+	length = 4096;
+	buffer = malloc(length);
+	read_offset = 0;
+
+	if (!buffer) {
+		err = errno;
+		goto out;
+	}
+
+	while (1) {
+		if ((rc = read(fd, buffer + read_offset, length - read_offset)) < 0) {
+			err = errno;
+			goto out;
+		}
+
+		if (rc == 0)
+			break;
+
+		read_offset += rc;
+
+		if (read_offset > length / 2) {
+			char *next_buffer;
+
+			length *= 2;
+			next_buffer = realloc(buffer, length);
+			if (!next_buffer) {
+				err = errno;
+				free(buffer);
+				goto out;
+			}
+			buffer = next_buffer;
+		}
+	}
+
+	*buf = buffer;
+	if (len)
+		*len = read_offset;
+
+	err = 0;
+
+out:
+	close(fd);
+	return err;
+}
+
 int write_file(const char *path, const char *buf, size_t count)
 {
 	int fd;
-- 
2.38.1

Re: [PATCH v3 2/7] selftests/powerpc: Add ptrace setup_core_pattern() null-terminator

[Andrew Donnellan]

On Mon, 2022-11-28 at 15:19 +1100, Benjamin Gray wrote:
> - malloc() does not zero the buffer,
> - fread() does not null-terminate it's output,
> - `cat /proc/sys/kernel/core_pattern | hexdump -C` shows the file is
>   not inherently null-terminated
> 
> So using string operations on the buffer is risky. Explicitly add a
> null
> character to the end to make it safer.
> 
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
>  tools/testing/selftests/powerpc/ptrace/core-pkey.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> index bbc05ffc5860..5c82ed9e7c65 100644
> --- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> +++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> @@ -383,7 +383,7 @@ static int setup_core_pattern(char
> **core_pattern_, bool *changed_)
>                 goto out;
>         }
>  
> -       ret = fread(core_pattern, 1, PATH_MAX, f);
> +       ret = fread(core_pattern, 1, PATH_MAX - 1, f);
>         fclose(f);
>         if (!ret) {
>                 perror("Error reading core_pattern file");
> @@ -391,6 +391,8 @@ static int setup_core_pattern(char
> **core_pattern_, bool *changed_)
>                 goto out;
>         }
>  
> +       core_pattern[ret] = '\0';
> +
>         /* Check whether we can predict the name of the core file. */
>         if (!strcmp(core_pattern, "core") || !strcmp(core_pattern,
> "core.%p"))
>                 *changed_ = false;

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH v3 3/7] selftests/powerpc: Add generic read/write file util

[Michael Ellerman]

Benjamin Gray <bgray@linux.ibm.com> writes:
> File read/write is reimplemented in about 5 different ways in the
> various PowerPC selftests. This indicates it should be a common util.
>
> Add a common read_file / write_file implementation and convert users
> to it where (easily) possible.
>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
>  tools/testing/selftests/powerpc/dscr/dscr.h   |  36 ++----
>  .../selftests/powerpc/dscr/dscr_sysfs_test.c  |  19 +--
>  .../testing/selftests/powerpc/include/utils.h |   2 +
>  .../selftests/powerpc/nx-gzip/gzfht_test.c    |  49 +++-----
>  tools/testing/selftests/powerpc/pmu/lib.c     |  27 +----
>  .../selftests/powerpc/ptrace/core-pkey.c      |  30 ++---
>  tools/testing/selftests/powerpc/utils.c       | 108 ++++++++++--------
>  7 files changed, 107 insertions(+), 164 deletions(-)
>
> diff --git a/tools/testing/selftests/powerpc/dscr/dscr.h b/tools/testing/selftests/powerpc/dscr/dscr.h
> index b703714e7d98..9a69d473ffdf 100644
> --- a/tools/testing/selftests/powerpc/dscr/dscr.h
> +++ b/tools/testing/selftests/powerpc/dscr/dscr.h
> @@ -64,48 +64,30 @@ inline void set_dscr_usr(unsigned long val)
>  /* Default DSCR access */
>  unsigned long get_default_dscr(void)
>  {
> -	int fd = -1, ret;
> -	char buf[16];
> +	int err;
> +	char buf[16] = {0};
>  	unsigned long val;
>  
> -	if (fd == -1) {
> -		fd = open(DSCR_DEFAULT, O_RDONLY);
> -		if (fd == -1) {
> -			perror("open() failed");
> -			exit(1);
> -		}
> -	}
> -	memset(buf, 0, sizeof(buf));
> -	lseek(fd, 0, SEEK_SET);
> -	ret = read(fd, buf, sizeof(buf));
> -	if (ret == -1) {
> -		perror("read() failed");
> +	if ((err = read_file(DSCR_DEFAULT, buf, sizeof(buf) - 1, NULL))) {
> +		fprintf(stderr, "get_default_dscr() read failed: %s\n", strerror(err));

I don't particularly like doing the assignment to err in the if.

And checkpatch flags it as an error, which means even if we did like it
we'd be setting ourselves up for a stream of fixup patches :)

So please just do the assignment and the if separately.

cheers

Re: (subset) [PATCH v3 0/7] Expand selftest utils

[Michael Ellerman]

On Mon, 28 Nov 2022 15:19:41 +1100, Benjamin Gray wrote:
> Started this when writing tests for a feature I'm working on, needing a way to
> read/write numbers to system files. After writing some utils to safely handle
> file IO and parsing, I realised I'd made the ~6th file read/write implementation
> and only(?) number parser that checks all the failure modes when expecting to
> parse a single number from a file.
> 
> So these utils ended up becoming this series. I also modified some other test
> utils I came across while doing so. My understanding is selftests are not expected
> to be backported, so I wasn't concerned about only introducing new utils and leaving
> the existing implementations be.
> 
> [...]

Patches 1 & 2 applied to powerpc/next.

[1/7] selftests/powerpc: Use mfspr/mtspr macros
      https://git.kernel.org/powerpc/c/aecfd680099ba518c34dff2941017c5aa97def52
[2/7] selftests/powerpc: Add ptrace setup_core_pattern() null-terminator
      https://git.kernel.org/powerpc/c/94ba4f2c33f42dae7813dc169a177e922a39560c

cheers

Re: [PATCH v3 3/7] selftests/powerpc: Add generic read/write file util

[Andrew Donnellan]

On Mon, 2022-11-28 at 15:19 +1100, Benjamin Gray wrote:
> File read/write is reimplemented in about 5 different ways in the
> various PowerPC selftests. This indicates it should be a common util.
> 
> Add a common read_file / write_file implementation and convert users
> to it where (easily) possible.
> 
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
>  tools/testing/selftests/powerpc/dscr/dscr.h   |  36 ++----
>  .../selftests/powerpc/dscr/dscr_sysfs_test.c  |  19 +--
>  .../testing/selftests/powerpc/include/utils.h |   2 +
>  .../selftests/powerpc/nx-gzip/gzfht_test.c    |  49 +++-----
>  tools/testing/selftests/powerpc/pmu/lib.c     |  27 +----
>  .../selftests/powerpc/ptrace/core-pkey.c      |  30 ++---
>  tools/testing/selftests/powerpc/utils.c       | 108 ++++++++++------
> --
>  7 files changed, 107 insertions(+), 164 deletions(-)
> 
> diff --git a/tools/testing/selftests/powerpc/dscr/dscr.h
> b/tools/testing/selftests/powerpc/dscr/dscr.h
> index b703714e7d98..9a69d473ffdf 100644
> --- a/tools/testing/selftests/powerpc/dscr/dscr.h
> +++ b/tools/testing/selftests/powerpc/dscr/dscr.h
> @@ -64,48 +64,30 @@ inline void set_dscr_usr(unsigned long val)
>  /* Default DSCR access */
>  unsigned long get_default_dscr(void)
>  {
> -       int fd = -1, ret;
> -       char buf[16];
> +       int err;
> +       char buf[16] = {0};
>         unsigned long val;
>  
> -       if (fd == -1) {
> -               fd = open(DSCR_DEFAULT, O_RDONLY);
> -               if (fd == -1) {
> -                       perror("open() failed");
> -                       exit(1);
> -               }
> -       }
> -       memset(buf, 0, sizeof(buf));
> -       lseek(fd, 0, SEEK_SET);
> -       ret = read(fd, buf, sizeof(buf));
> -       if (ret == -1) {
> -               perror("read() failed");
> +       if ((err = read_file(DSCR_DEFAULT, buf, sizeof(buf) - 1,
> NULL))) {

As mpe says, avoid assignment in conditionals.

> +               fprintf(stderr, "get_default_dscr() read failed:
> %s\n", strerror(err));
>                 exit(1);
>         }
> +
>         sscanf(buf, "%lx", &val);
> -       close(fd);
>         return val;
>  }
>  
>  void set_default_dscr(unsigned long val)
>  {
> -       int fd = -1, ret;
> +       int err;
>         char buf[16];
>  
> -       if (fd == -1) {
> -               fd = open(DSCR_DEFAULT, O_RDWR);
> -               if (fd == -1) {
> -                       perror("open() failed");
> -                       exit(1);
> -               }
> -       }
>         sprintf(buf, "%lx\n", val);
> -       ret = write(fd, buf, strlen(buf));
> -       if (ret == -1) {
> -               perror("write() failed");
> +
> +       if ((err = write_file(DSCR_DEFAULT, buf, strlen(buf)))) {
> +               fprintf(stderr, "set_default_dscr() write failed:
> %s\n", strerror(err));
>                 exit(1);
>         }
> -       close(fd);
>  }
>  
>  double uniform_deviate(int seed)
> diff --git a/tools/testing/selftests/powerpc/dscr/dscr_sysfs_test.c
> b/tools/testing/selftests/powerpc/dscr/dscr_sysfs_test.c
> index fbbdffdb2e5d..310946262a24 100644
> --- a/tools/testing/selftests/powerpc/dscr/dscr_sysfs_test.c
> +++ b/tools/testing/selftests/powerpc/dscr/dscr_sysfs_test.c
> @@ -12,23 +12,12 @@
>  
>  static int check_cpu_dscr_default(char *file, unsigned long val)
>  {
> -       char buf[10];
> -       int fd, rc;
> +       char buf[10] = {0};
> +       int rc;
>  
> -       fd = open(file, O_RDWR);
> -       if (fd == -1) {
> -               perror("open() failed");
> -               return 1;
> -       }
> -
> -       rc = read(fd, buf, sizeof(buf));
> -       if (rc == -1) {
> -               perror("read() failed");
> -               return 1;
> -       }
> -       close(fd);
> +       if ((rc = read_file(file, buf, sizeof(buf) - 1, NULL)))
> +               return rc;

As above

>  
> -       buf[rc] = '\0';
>         if (strtol(buf, NULL, 16) != val) {
>                 printf("DSCR match failed: %ld (system) %ld (cpu)\n",
>                                         val, strtol(buf, NULL, 16));
> diff --git a/tools/testing/selftests/powerpc/include/utils.h
> b/tools/testing/selftests/powerpc/include/utils.h
> index e222a5858450..70885e5814a8 100644
> --- a/tools/testing/selftests/powerpc/include/utils.h
> +++ b/tools/testing/selftests/powerpc/include/utils.h
> @@ -33,6 +33,8 @@ void *get_auxv_entry(int type);
>  
>  int pick_online_cpu(void);
>  
> +int read_file(const char *path, char *buf, size_t count, size_t
> *len);
> +int write_file(const char *path, const char *buf, size_t count);
>  int read_debugfs_file(char *debugfs_file, int *result);
>  int write_debugfs_file(char *debugfs_file, int result);
>  int read_sysfs_file(char *debugfs_file, char *result, size_t
> result_size);
> diff --git a/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
> b/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
> index 095195a25687..a6a226e1b8ba 100644
> --- a/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
> +++ b/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
> @@ -146,49 +146,36 @@ int gzip_header_blank(char *buf)
>  /* Caller must free the allocated buffer return nonzero on error. */
>  int read_alloc_input_file(char *fname, char **buf, size_t *bufsize)
>  {
> +       int err;
>         struct stat statbuf;
> -       FILE *fp;
>         char *p;
>         size_t num_bytes;
>  
>         if (stat(fname, &statbuf)) {
>                 perror(fname);
> -               return(-1);
> -       }
> -       fp = fopen(fname, "r");
> -       if (fp == NULL) {
> -               perror(fname);
> -               return(-1);
> +               return -1;
>         }
> +
>         assert(NULL != (p = (char *) malloc(statbuf.st_size)));
> -       num_bytes = fread(p, 1, statbuf.st_size, fp);
> -       if (ferror(fp) || (num_bytes != statbuf.st_size)) {
> -               perror(fname);
> -               return(-1);
> +
> +       if ((err = read_file(fname, p, statbuf.st_size, &num_bytes)))

As above

> {
> +               fprintf(stderr, "Failed to read file: %s\n",
> strerror(err));
> +               goto fail;
>         }
> +
> +       if (num_bytes != statbuf.st_size) {
> +               fprintf(stderr, "Actual bytes != expected bytes\n");
> +               err = -1;
> +               goto fail;
> +       }
> +
>         *buf = p;
>         *bufsize = num_bytes;
>         return 0;
> -}
>  
> -/* Returns nonzero on error */
> -int write_output_file(char *fname, char *buf, size_t bufsize)
> -{
> -       FILE *fp;
> -       size_t num_bytes;
> -
> -       fp = fopen(fname, "w");
> -       if (fp == NULL) {
> -               perror(fname);
> -               return(-1);
> -       }
> -       num_bytes = fwrite(buf, 1, bufsize, fp);
> -       if (ferror(fp) || (num_bytes != bufsize)) {
> -               perror(fname);
> -               return(-1);
> -       }
> -       fclose(fp);
> -       return 0;
> +fail:
> +       free(p);
> +       return err;
>  }
>  
>  /*
> @@ -399,7 +386,7 @@ int compress_file(int argc, char **argv, void
> *handle)
>         assert(FNAME_MAX > (strlen(argv[1]) + strlen(FEXT)));
>         strcpy(outname, argv[1]);
>         strcat(outname, FEXT);
> -       if (write_output_file(outname, outbuf, dsttotlen)) {
> +       if (write_file(outname, outbuf, dsttotlen)) {
>                 fprintf(stderr, "write error: %s\n", outname);
>                 exit(-1);
>         }
> diff --git a/tools/testing/selftests/powerpc/pmu/lib.c
> b/tools/testing/selftests/powerpc/pmu/lib.c
> index 88690b97b7b9..e8960e7a1271 100644
> --- a/tools/testing/selftests/powerpc/pmu/lib.c
> +++ b/tools/testing/selftests/powerpc/pmu/lib.c
> @@ -190,38 +190,21 @@ int parse_proc_maps(void)
>  
>  bool require_paranoia_below(int level)
>  {
> +       int err;
>         long current;
>         char *end, buf[16];
> -       FILE *f;
> -       bool rc;
> -
> -       rc = false;
> -
> -       f = fopen(PARANOID_PATH, "r");
> -       if (!f) {
> -               perror("fopen");
> -               goto out;
> -       }
>  
> -       if (!fgets(buf, sizeof(buf), f)) {
> +       if ((err = read_file(PARANOID_PATH, buf, sizeof(buf), NULL)))

As above

> {
>                 printf("Couldn't read " PARANOID_PATH "?\n");
> -               goto out_close;
> +               return false;
>         }
>  
>         current = strtol(buf, &end, 10);
>  
>         if (end == buf) {
>                 printf("Couldn't parse " PARANOID_PATH "?\n");
> -               goto out_close;
> +               return false;
>         }
>  
> -       if (current >= level)
> -               goto out_close;
> -
> -       rc = true;
> -out_close:
> -       fclose(f);
> -out:
> -       return rc;
> +       return current < level;
>  }
> -
> diff --git a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> index 5c82ed9e7c65..46e0695ed8b0 100644
> --- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> +++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> @@ -348,16 +348,11 @@ static int parent(struct shared_info *info,
> pid_t pid)
>  
>  static int write_core_pattern(const char *core_pattern)
>  {
> -       size_t len = strlen(core_pattern), ret;
> -       FILE *f;
> +       int err;
>  
> -       f = fopen(core_pattern_file, "w");
> -       SKIP_IF_MSG(!f, "Try with root privileges");
> -
> -       ret = fwrite(core_pattern, 1, len, f);
> -       fclose(f);
> -       if (ret != len) {
> -               perror("Error writing to core_pattern file");
> +       if ((err = write_file(core_pattern_file, core_pattern,
> strlen(core_pattern)))) {

As above

> +               SKIP_IF_MSG(err == -EPERM, "Try with root
> privileges");
> +               fprintf(stderr, "Error writing core_pattern file:
> %s\n", strerror(err));
>                 return TEST_FAIL;
>         }
>  
> @@ -366,8 +361,8 @@ static int write_core_pattern(const char
> *core_pattern)
>  
>  static int setup_core_pattern(char **core_pattern_, bool *changed_)
>  {
> -       FILE *f;
>         char *core_pattern;
> +       size_t len;
>         int ret;
>  
>         core_pattern = malloc(PATH_MAX);
> @@ -376,22 +371,13 @@ static int setup_core_pattern(char
> **core_pattern_, bool *changed_)
>                 return TEST_FAIL;
>         }
>  
> -       f = fopen(core_pattern_file, "r");
> -       if (!f) {
> -               perror("Error opening core_pattern file");
> -               ret = TEST_FAIL;
> -               goto out;
> -       }
> -
> -       ret = fread(core_pattern, 1, PATH_MAX - 1, f);
> -       fclose(f);
> -       if (!ret) {
> -               perror("Error reading core_pattern file");
> +       if ((ret = read_file(core_pattern_file, core_pattern,
> PATH_MAX - 1, &len))) {

As above

> +               fprintf(stderr, "Error reading core_pattern file:
> %s\n", strerror(ret));
>                 ret = TEST_FAIL;
>                 goto out;
>         }
>  
> -       core_pattern[ret] = '\0';
> +       core_pattern[len] = '\0';
>  
>         /* Check whether we can predict the name of the core file. */
>         if (!strcmp(core_pattern, "core") || !strcmp(core_pattern,
> "core.%p"))
> diff --git a/tools/testing/selftests/powerpc/utils.c
> b/tools/testing/selftests/powerpc/utils.c
> index 1f36ee1a909a..861b1f7aa95f 100644
> --- a/tools/testing/selftests/powerpc/utils.c
> +++ b/tools/testing/selftests/powerpc/utils.c
> @@ -26,34 +26,73 @@
>  
>  static char auxv[4096];
>  
> -int read_auxv(char *buf, ssize_t buf_size)
> +int read_file(const char *path, char *buf, size_t count, size_t
> *len)

Could make this more read(2) like by returning the bytes read as
ssize_t, but this is also fine

>  {
> -       ssize_t num;
> -       int rc, fd;
> +       ssize_t rc;
> +       int fd;
> +       int err;
> +       char eof;
> +
> +       if ((fd = open(path, O_RDONLY)) < 0)

As above

> +               return errno;
>  
> -       fd = open("/proc/self/auxv", O_RDONLY);
> -       if (fd == -1) {
> -               perror("open");
> -               return -errno;
> +       if ((rc = read(fd, buf, count)) < 0) {

As above

> +               err = errno;
> +               goto out;
>         }
>  
> -       num = read(fd, buf, buf_size);
> -       if (num < 0) {
> -               perror("read");
> -               rc = -EIO;
> +       if (len)
> +               *len = rc;
> +
> +       /* Overflow if there are still more bytes after filling the
> buffer */
> +       if (rc == count && (rc = read(fd, &eof, 1)) != 0) {

As above - this also needs to be split into two conditionals IMHO

> +               err = EOVERFLOW;
>                 goto out;
>         }
>  
> -       if (num > buf_size) {
> -               printf("overflowed auxv buffer\n");
> -               rc = -EOVERFLOW;
> +       err = 0;
> +
> +out:
> +       close(fd);
> +       return err;
> +}
> +
> +int write_file(const char *path, const char *buf, size_t count)
> +{
> +       int fd;
> +       int err;
> +       ssize_t rc;
> +
> +       if ((fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0644)) <
> 0)
> +               return errno;
> +
> +       if ((rc = write(fd, buf, count)) < 0) {
> +               err = errno;
>                 goto out;
>         }
>  
> -       rc = 0;
> +       if (rc != count) {
> +               err = EOVERFLOW;
> +               goto out;
> +       }
> +
> +       err = 0;
> +
>  out:
>         close(fd);
> -       return rc;
> +       return err;
> +}
> +
> +int read_auxv(char *buf, ssize_t buf_size)
> +{
> +       int err;
> +
> +       if ((err = read_file("/proc/self/auxv", buf, buf_size,
> NULL))) {

As above

> +               fprintf(stderr, "Error reading auxv: %s\n",
> strerror(err));
> +               return err;
> +       }
> +
> +       return 0;
>  }
>  
>  void *find_auxv_entry(int type, char *auxv)
> @@ -142,65 +181,40 @@ bool is_ppc64le(void)
>  int read_sysfs_file(char *fpath, char *result, size_t result_size)
>  {
>         char path[PATH_MAX] = "/sys/";
> -       int rc = -1, fd;
>  
>         strncat(path, fpath, PATH_MAX - strlen(path) - 1);
>  
> -       if ((fd = open(path, O_RDONLY)) < 0)
> -               return rc;
> -
> -       rc = read(fd, result, result_size);
> -
> -       close(fd);
> -
> -       if (rc < 0)
> -               return rc;
> -
> -       return 0;
> +       return read_file(path, result, result_size, NULL);
>  }
>  
>  int read_debugfs_file(char *debugfs_file, int *result)
>  {
> -       int rc = -1, fd;
> +       int err;
>         char path[PATH_MAX];
> -       char value[16];
> +       char value[16] = {0};
>  
>         strcpy(path, "/sys/kernel/debug/");
>         strncat(path, debugfs_file, PATH_MAX - strlen(path) - 1);
>  
> -       if ((fd = open(path, O_RDONLY)) < 0)
> -               return rc;
> -
> -       if ((rc = read(fd, value, sizeof(value))) < 0)
> -               return rc;
> +       if ((err = read_file(path, value, sizeof(value) - 1, NULL)))
> +               return err;
>  
> -       value[15] = 0;
>         *result = atoi(value);
> -       close(fd);
>  
>         return 0;
>  }
>  
>  int write_debugfs_file(char *debugfs_file, int result)
>  {
> -       int rc = -1, fd;
>         char path[PATH_MAX];
>         char value[16];
>  
>         strcpy(path, "/sys/kernel/debug/");
>         strncat(path, debugfs_file, PATH_MAX - strlen(path) - 1);
>  
> -       if ((fd = open(path, O_WRONLY)) < 0)
> -               return rc;
> -
>         snprintf(value, 16, "%d", result);
>  
> -       if ((rc = write(fd, value, strlen(value))) < 0)
> -               return rc;
> -
> -       close(fd);
> -
> -       return 0;
> +       return write_file(path, value, strlen(value));
>  }
>  
>  static long perf_event_open(struct perf_event_attr *hw_event, pid_t
> pid,

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH v3 4/7] selftests/powerpc: Add read/write debugfs file, int

[Andrew Donnellan]

On Mon, 2022-11-28 at 15:19 +1100, Benjamin Gray wrote:
> Debugfs files are not always integers, so make *_file return/write a
> byte buffer, and *_int deal with int values specifically. This
> increases
> consistency with the other file read/write helpers.
> 
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

This seems like a sensible idea.

Nitpick below, otherwise it looks like you have changed over all 21
call sites and the new helpers look better.

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> -int read_debugfs_file(char *debugfs_file, int *result)
> +int read_debugfs_int(const char *debugfs_file, int *result)
>  {
>         int err;
> -       char path[PATH_MAX];
>         char value[16] = {0};
>  
> -       strcpy(path, "/sys/kernel/debug/");
> -       strncat(path, debugfs_file, PATH_MAX - strlen(path) - 1);
> -
> -       if ((err = read_file(path, value, sizeof(value) - 1, NULL)))
> +       if ((err = read_debugfs_file(debugfs_file, value,
> sizeof(value) - 1)))

Per my comments on the previous patch, separate the function call from
the error check

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited