Hi Ilpo,
On 12/11/2023 4:17 AM, Ilpo Järvinen wrote:
> The resctrl selftest code contains a number of perror() calls. Some of
> them come with hash character and some don't. The kselftest framework
> provides ksft_perror() that is compatible with test output formatting
> so it should be used instead of adding custom hash signs.
>
> Some perror() calls are too far away from anything that sets error.
> For those call sites, ksft_print_msg() must be used instead.
>
> Convert perror() to ksft_perror() or ksft_print_msg().
>
> Other related changes:
> - Remove hash signs
> - Remove trailing stops & newlines from ksft_perror()
> - Add terminating newlines for converted ksft_print_msg()
> - Use consistent capitalization
>
Another great cleanup. Also thanks for fixing some non-sensical messages.
...
> @@ -149,7 +149,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
> param.num_of_runs = 0;
>
> if (pipe(pipefd)) {
> - perror("# Unable to create pipe");
> + ksft_perror("Unable to create pipe");
> return errno;
> }
>
> @@ -185,7 +185,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
> * Just print the error message.
> * Let while(1) run and wait for itself to be killed.
> */
> - perror("# failed signaling parent process");
> + ksft_perror("Failed signaling parent process");
>
Partial writes are not actually errors and it cannot be expected that errno be
set
in these cases. In these cases I think ksft_print_msg() would be more
appropriate.
> close(pipefd[1]);
> while (1)
> @@ -197,7 +197,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
> while (pipe_message != 1) {
> if (read(pipefd[0], &pipe_message,
> sizeof(pipe_message)) < sizeof(pipe_message)) {
> - perror("# failed reading from child process");
> + ksft_perror("Failed reading from child
> process");
> break;
> }
Same with partial reads.
...
> @@ -540,14 +540,14 @@ static int print_results_bw(char *filename, int
> bm_pid, float bw_imc,
> } else {
> fp = fopen(filename, "a");
> if (!fp) {
> - perror("Cannot open results file");
> + ksft_perror("Cannot open results file");
>
> return errno;
> }
> if (fprintf(fp, "Pid: %d \t Mem_BW_iMC: %f \t Mem_BW_resc: %lu
> \t Difference: %lu\n",
> bm_pid, bw_imc, bw_resc, diff) <= 0) {
> + ksft_perror("Could not log results");
> fclose(fp);
> - perror("Could not log results.");
>
> return errno;
>From what I can tell fprintf() does not set errno on error. Perhaps this
should rather be ksft_print_msg()?
...
> @@ -738,15 +743,17 @@ int resctrl_val(const char * const *benchmark_cmd,
> struct resctrl_val_param *par
> sigact.sa_flags = SA_SIGINFO;
>
> /* Register for "SIGUSR1" signal from parent */
> - if (sigaction(SIGUSR1, &sigact, NULL))
> - PARENT_EXIT("Can't register child for signal");
> + if (sigaction(SIGUSR1, &sigact, NULL)) {
> + ksft_perror("Can't register child for signal");
> + PARENT_EXIT();
> + }
>
> /* Tell parent that child is ready */
> close(pipefd[0]);
> pipe_message = 1;
> if (write(pipefd[1], &pipe_message, sizeof(pipe_message)) <
> sizeof(pipe_message)) {
> - perror("# failed signaling parent process");
> + ksft_perror("Failed signaling parent process");
> close(pipefd[1]);
> return -1;
another partial write possibility
> }
> @@ -755,7 +762,8 @@ int resctrl_val(const char * const *benchmark_cmd, struct
> resctrl_val_param *par
> /* Suspend child until delivery of "SIGUSR1" from parent */
> sigsuspend(&sigact.sa_mask);
>
> - PARENT_EXIT("Child is done");
> + ksft_perror("Child is done");
> + PARENT_EXIT();
> }
>
> ksft_print_msg("Benchmark PID: %d\n", bm_pid);
> @@ -796,7 +804,7 @@ int resctrl_val(const char * const *benchmark_cmd, struct
> resctrl_val_param *par
> while (pipe_message != 1) {
> if (read(pipefd[0], &pipe_message, sizeof(pipe_message)) <
> sizeof(pipe_message)) {
> - perror("# failed reading message from child process");
> + ksft_perror("Failed reading message from child
> process");
> close(pipefd[0]);
> goto out;
another partial read possibility
...
> @@ -348,12 +348,12 @@ static int write_pid_to_tasks(char *tasks, pid_t pid)
>
> fp = fopen(tasks, "w");
> if (!fp) {
> - perror("Failed to open tasks file");
> + ksft_perror("Failed to open tasks file");
>
> return -1;
> }
> if (fprintf(fp, "%d\n", pid) < 0) {
> - perror("Failed to wr pid to tasks file");
> + ksft_perror("Failed to wr pid to tasks file");
> fclose(fp);
>
another fprintf() that I do not think sets errno on error.
Reinette
