Re: [PATCH v3 01/29] selftests/resctrl: Convert perror() to ksft_perror() or ksft_print_msg()

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