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

[Reinette Chatre <reinette.chatre@intel.com>]

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