linux-kernel - Re: [PATCH v3 01/29] selftests/resctrl: Convert perror() to ksft_perror() or ksft_print

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1e7ede83-ac80-43aa-a452-0f95b32d849c@intel.com>
Date:   Wed, 13 Dec 2023 13:53:55 -0800
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        <linux-kselftest@...r.kernel.org>, Shuah Khan <shuah@...nel.org>,
        "Shaopeng Tan" <tan.shaopeng@...fujitsu.com>,
        Maciej Wieczór-Retman 
        <maciej.wieczor-retman@...el.com>,
        Fenghua Yu <fenghua.yu@...el.com>
CC:     <linux-kernel@...r.kernel.org>
Subject: 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