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 <bg...@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
a...@linux.ibm.com IBM Australia Limited
