Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <1e7ede83-ac80-43aa-a452-0f95b32d849c@intel.com>
Date:   Wed, 13 Dec 2023 13:53:55 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 01/29] selftests/resctrl: Convert perror() to
 ksft_perror() or ksft_print_msg()
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        <linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>,
        "Shaopeng Tan" <tan.shaopeng@jp.fujitsu.com>,
        =?UTF-8?Q?Maciej_Wiecz=C3=B3r-Retman?= 
        <maciej.wieczor-retman@intel.com>,
        Fenghua Yu <fenghua.yu@intel.com>
CC:     <linux-kernel@vger.kernel.org>
References: <20231211121826.14392-1-ilpo.jarvinen@linux.intel.com>
 <20231211121826.14392-2-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20231211121826.14392-2-ilpo.jarvinen@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NmpnVkMrZDMyN0QwVktnY2tRMGx0dE9yS2V5cWRkYzd4dFpDSkZ1WlAwMGVv?=
 =?utf-8?B?WllLeCtMSzZqYVpsaUkzQk1oYlI1LzVHMVE4ZmV6SFRiM1FySGVoSmdzbmk1?=
 =?utf-8?B?NDdyWitQMVNpSzlnTUwrTHo2Skt3VDZoeXN6Nnc5RXc2TVhBbUpQQWhkeWM0?=
 =?utf-8?B?UHhncnJTNXN5R3NQQTZXbXZEVENFa2lnYXJqLzZsb0NMQnBsTjdUL0MzeVNG?=
 =?utf-8?B?cTJHanhmUTU4YXlrT0hHVUczbkQwZGhBR0x3RkRmcnFpcFNUcjNtcUcxK1J1?=
 =?utf-8?B?eU81azdiQ3ZiV2E4eUtQc3pRWkJCZ0dLbkFvSmNtWld0bmtqZXFPSllBRktx?=
 =?utf-8?B?bWRQZVAzb2xqa29CMCsvTDR6OC9Jblp5SEVTT0xvUmdOVXAyMFJjbnRjZ3Zh?=
 =?utf-8?B?ZnN3TElFNlc5eEJqTWhlczJzRmhGakdLTmM2eUdyMzNLRlhyM1E2VUxiUmhS?=
 =?utf-8?B?QzJOSXB5a1lHOGF5b1JQMW9velJMRklUMksvK3h4V3B5MDNnakcxNk5IL1ZW?=
 =?utf-8?B?ZFg4M0RFd29wVzEzRUZ4aUZqL25yeVRvSytpNk53Y0VpWTlxdCtkem5yaXV0?=
 =?utf-8?B?bHlOUDAzTWFremNUZkhaV0lPLy9kdkplcmdhZWNRQXJ5RE9vVnByUktqSlhF?=
 =?utf-8?B?YW9PZ3YyQUNSNCs1Ri9LaHd2K3J3bTNYL2lLVG41SytxdTBMTHZsWU9TdFhM?=
 =?utf-8?B?WVFYOGJ4YVU3b3Q4a3FMMGRXQ0c2bXpzTTRCQ0MzNWU5cGQ3NlRvODNNS1Vx?=
 =?utf-8?B?Q0ZBT2dpT2xtVmkrSU5xRWxURXdqbW1TcnBEdENZT28xNTNYNFBlMnN4UnF1?=
 =?utf-8?B?YlNqNnA3RUoxTVJkZUtRMENmdmtRTlR0bjNuNmkwMnRlS3U3WEk3QjZ2M2lo?=
 =?utf-8?B?eGtRQWorTm5TcExxRDh2SXh0bFpBd01DZ3pHYkVuR0V2V3RMMkNiQ3prUTla?=
 =?utf-8?B?d1U0dlNtQ1grMzJtUW5XWHkyNldKbVRzaDJDSEV4TnVMSThhcEVqVi9MUVNm?=
 =?utf-8?B?WThjTnhLRWFvaVM4SUNkZ2JaeHpySGtZaXUrMEdTOXdpcGk1ODVBTGdwdG9D?=
 =?utf-8?B?N1lZc253S0RKNU5NeU84dC9XcllSV3dpdTBzcEtXcWljUjFDUmRSSm0rNzFX?=
 =?utf-8?B?WmRTakhOMVZkZzFjMHBENXRnbXNvMU9qYkthYzI3MUVVMCtFb01DV0FrbFJG?=
 =?utf-8?B?eGpyditweGc1ZUNLZzNOSVdBK3dSSGluQVIxMHdMUUg5bUVhNi9zeE4wUy9s?=
 =?utf-8?B?MndQYnI3RlNmYUh4ejdlR01JNUYyVENLVDAvaEs0aTZoMTdIYi90YWpSWUlp?=
 =?utf-8?B?U2wrNE9CUm5JakhuWGdKOUhkTTBvT21wb1BxVGhvTWoxV3kwVW1wRlVVTjJo?=
 =?utf-8?B?ekltaEJiUGllRWROK0JCdW12NkVNMjd4NHBXNnlMekl0VWFBWURoWjBkcVN4?=
 =?utf-8?B?TEJoTVRhZHRDUjk2eHF1N3htRWZkellxNzV5VEJTbkVBSGdwZnoxcUxDZnJR?=
 =?utf-8?B?amRTUkNxMWhGN1ZHZnM0RzVvSWt2M1ZvRjFhL2NEZDZQei9VOEcyK1IrZXpT?=
 =?utf-8?B?YnhiekJ2YWMwY2hJeVRhMStCNFpsRGN3ZFJaSFNOeVpLQitLajJ5bGxBbmVh?=
 =?utf-8?B?WFJWY0NxUWVoL0dLaE1BblJ2SDNCZ2YzYVhuZzNOTWU2MEc5Z2FaamtDOEQ4?=
 =?utf-8?B?bEJUM3lldTl4ZkZEQ0s1dG1BWkJ0M3pGK0xOWTVwL2NPT1FXbmpxMER0dDF2?=
 =?utf-8?B?QUFWYk1aWWs3SlV5QXlQMXduV01vZEFUYVRqZG0wc0JkMU5yYWZ0enlNWVhX?=
 =?utf-8?B?dmw1QjQycGE2UllFR0ZhdlJGUXU2UnI4ZkhUekprdjE1OHlWdmluS04vVDlT?=
 =?utf-8?B?WGl2a3BkMklvUnppNm83bTVHNTYrR1k2SHBPNmFSR0lzMjBIb3hRa2J1c29N?=
 =?utf-8?B?dHoyNCtFbGJablh6VElTbnJTVHFBcGdhVDNjcmNqc0JiZWFYYWp6WC9zT2pY?=
 =?utf-8?B?Y0JWekc1U3VuNUpqRjhTK1ZzMGMvYnpVMHdPZzVIZksrV0F3K3lsUCtTQ1lo?=
 =?utf-8?B?SFp5d1I2dkhENjdQcE5SY0x2RkFLWjFrSnpiU1RETFB2cGdxSlFBTTVyKzgy?=
 =?utf-8?B?SWFIemFQM1FKcUVCQXp0cFFxNlljUVozNEVmQmgwd051QkRXWEJGeWlQWk1K?=
 =?utf-8?B?emc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 72718896-2737-4dc0-586d-08dbfc2601d0
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Dec 2023 21:53:57.4102
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: vcy02K6k73P+LpB3iZhlC+pODy6vAWn5OAzXDwH4it6yAsVnoJAFWhjpMyaGRVoouGAbBxDbtl37Xkhe6m5RU8NOlfOkeXEGDt0VUqnVWjQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR11MB7581
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Wed, 13 Dec 2023 13:54:20 -0800 (PST)

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