Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Tue, 12 Sep 2023 08:32:57 +0200
From:   Maciej =?utf-8?Q?Wiecz=C3=B3r-Retman?= 
        <maciej.wieczor-retman@intel.com>
To:     Reinette Chatre <reinette.chatre@intel.com>
CC:     Fenghua Yu <fenghua.yu@intel.com>, Shuah Khan <shuah@kernel.org>,
        <ilpo.jarvinen@linux.intel.com>, <linux-kernel@vger.kernel.org>,
        <linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH RESEND v3 1/2] selftests/resctrl: Fix schemata write
 error check
Message-ID: <sr6ana6d7ebtuxbhjuo6kcnhnn2zzvg3ivve6mndqeb3nxrzo7@mrfmtzlxlwdh>
References: <cover.1693575451.git.maciej.wieczor-retman@intel.com>
 <960b5302cee8e5bb0e83dcf20cd2ef4d353b7b0b.1693575451.git.maciej.wieczor-retman@intel.com>
 <99eabc5c-f5e3-27dd-0a29-ad0cdb7b4239@intel.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <99eabc5c-f5e3-27dd-0a29-ad0cdb7b4239@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cTBFYWNUNkFhWTYxWnFMMkhZZGNlUWtIU2tLTHRvTUswYmprK1A1NkdYVE96?=
 =?utf-8?B?L3BKTkVDbkhqbXEzSHpQTGxZaFNuN2hjTll1aDJJREwrUzBPbFRYVVpHeHNN?=
 =?utf-8?B?QzI0aDdaYm95c3NobVBJa2xWME5hRzJKS1FhaFR0Z3FITkJNNmtBQTAwaUQ5?=
 =?utf-8?B?NHhUQWszK0t5R2hkSHM3TWE2VURIWXRCUmcxOW9Gb28wM1JvS3pFaHgxRmpT?=
 =?utf-8?B?OTlWdHVQWXRKT3VPM3lsenJtSDRQaytCM1N4eTJFaDBmNHcrSUh3c2RrZlVV?=
 =?utf-8?B?akFjMVlhWERyeFJhVFI3U2wyek1kRm1VYngrSEhlVjBpeEdqQUNIZmhJMTkr?=
 =?utf-8?B?OTZrbklhUGFRZWxWTytTL0t4azltNTZvblo1WVdSSlpSU0lPYVhURFcwcHU5?=
 =?utf-8?B?KzRvbW1IUm1TQS8rbkRzRFhHTmdEakZQSzJ3WUNvQ252Rk1OUU90dU02SW5j?=
 =?utf-8?B?a2NocVprWklMT3p0QUlrOVNOQ3NUTGY0QkdEUS9wNjh4cSsrdzhqUmpuOSs0?=
 =?utf-8?B?WWdFQTEyak5iTzYzNXA3ZmdOdjlKbHNvVWt2Ti92Y211SlZHalFDZCtnK3NZ?=
 =?utf-8?B?dG1TRmhqTVFsMWhObVpMdzZhT0t3NEFPQ2VOVzFTS25MK1NHcEhoQVU5VHR2?=
 =?utf-8?B?T0k3eTVjRXdpTTlOU2VOU3FRVGp1MDVST2ppUmZ1L0EzYjB5aWVoTjJyQnQv?=
 =?utf-8?B?VThtVmtuNjJISTg2S0kzY200WEozUjhVU3JWU29ZN21qc3JUMWNMeWhld29X?=
 =?utf-8?B?aTdORFJFaVdCV3N2YkFTb0RnTjBXeklFSWVHaktYMEF6R2NIU2NFZTZoZUsx?=
 =?utf-8?B?Y0JmdGlEbHA0UVZSMjNMZ1BOZkZla0tVdk9hQm9YMkdyS2tINGtKcU4wbTBM?=
 =?utf-8?B?Zkd3UmpiaitPVjFKRFNmZFV6cW1NekUxSE93STR1YVZJaHdOenJ6R1FDb0lH?=
 =?utf-8?B?VklqSUhpWnR2Z2Z3RWhaSWo1dkFKRlFseWg3eFhQSU0rR3NqSUFvNTJ3VVk4?=
 =?utf-8?B?WGEyMlB6d2QyaUdxSS9DbWZOeXcvanBYYVFpS2V4Zy9pSkJKcEQydzdPSHlK?=
 =?utf-8?B?cGtBOFQvdmxIMDV2V0VpUzJOemtaSFZsNU9SUW9mclpjeS9JemplYVIwelRY?=
 =?utf-8?B?N1pjMFhReExiR1JIMXhRRWlpTThySXdKOWkrVytOdU13MnhDZllPb2FReTRj?=
 =?utf-8?B?Wms0ZzYzRExNYjVnaDNQaWw0V1ptRGNMNG1TeE1yYW8vN21hUkdZbUVESEZI?=
 =?utf-8?B?KytuRnErY2k3dndUUzJuTmpsYURYY25WOU5NVlREaC9iRzFhUC90SE96dlNm?=
 =?utf-8?B?ZGs5UUlVVk9tVmMydzZGU3R4UndCVElyR2xTd1R0bzZLSlhCZWZKT3QzK1pt?=
 =?utf-8?B?azBLNXNFVlhwZVVqMWp2cmlPem1XTk4xVFJGT1dzN3Z2d0FTVjhnbFRiSmlm?=
 =?utf-8?B?OVR1MGxFbitUY2tNWkNzTWc5SmFxMitoWFVVVy8zSmdwYytkU3ZkQmkwRlVU?=
 =?utf-8?B?bzRxVmJWTFVIVVdqQVRxUFBTQUE5TW5zM3B2eVV4dFVjRnRFWm94RXdNVXpz?=
 =?utf-8?B?UTJFcmpRV1dRRTJkSFJLN1dVMjZXSzkwR1FlaTBvbytYZkRPTlQxRk9PcnZ4?=
 =?utf-8?B?Y3RzQmYydGhMTWo3QW5zVW5yd1MydW5nV28rNGdjZHZIa3gwc2NuRW8xU3VT?=
 =?utf-8?B?c1dFcXQzSDZ3K0NNMnlEcml4S2NMeWRiNWlNNXVrRUJ3SUVvMXYwNDJlemJi?=
 =?utf-8?B?UktwS1orcXBsYW1wcjl2NFRwZzNVN0pINkJZaGRrNHRjQVpQSFB0MlZtMHlL?=
 =?utf-8?B?citYeTZzYStpTXNJSXFkUEYyc1NIQ2RLTjZXT0ozYVFBSTM2R0xnNit5bDN2?=
 =?utf-8?B?dVhvL0poQ1RIdGRzUVNYa2ljYng1MVUvR3lrazR4dTk4UDVoa055WS9KM2sw?=
 =?utf-8?B?QjRjNkdGU0RENmNzZjhmRDNLT0hueUpZT0R5a0FXd01XNzcvOWc1VXhSLzJU?=
 =?utf-8?B?T1lhb3hNc2pudW1NOEVtaFRGMHRPck5aZ0ZHYTBrLzJHUjIyUnViWDVvVCt6?=
 =?utf-8?B?VmttNUYraW5Pejd3MVQ4cUNEUVZHYzRpd0dWNTdwemZoRzFUZ0YwemNWYWhO?=
 =?utf-8?B?aVNhYnM5WDFNMTNDcXVEdVRXd3FkVUtlWXV3MHRyR1gzZHIyT01XRVVucHcw?=
 =?utf-8?Q?a2VkfsNjnhmoWHtYsPAETRY=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 073b119a-9d10-422d-ac17-08dbb35a1e09
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6231.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Sep 2023 06:33:03.7119
 (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: tayqJoZx3dh5LFC1ckiLYqXGs2bUMNrg1/cC5teccmW7coLkLlGDAJMeta7l7hw4oelj9Bx/ikpv6tFBjchBw52JycY6B0Ms+A0N67e7NpM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7802
X-OriginatorOrg: intel.com
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 (pete.vger.email [0.0.0.0]); Mon, 11 Sep 2023 23:33:17 -0700 (PDT)
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 autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email

On 2023-09-11 at 09:59:06 -0700, Reinette Chatre wrote:
>Hi Maciej,
>When I build the tests with this applied I encounter the following:
>
>resctrlfs.c: In function ‘write_schemata’:
>resctrlfs.c:475:14: warning: implicit declaration of function ‘open’; did you mean ‘popen’? [-Wimplicit-function-declaration]
>  475 |         fd = open(controlgroup, O_WRONLY);
>      |              ^~~~
>      |              popen
>resctrlfs.c:475:33: error: ‘O_WRONLY’ undeclared (first use in this function)
>  475 |         fd = open(controlgroup, O_WRONLY);
>      |                                 ^~~~~~~~
>resctrlfs.c:475:33: note: each undeclared identifier is reported only once for each function it appears in

Hmm, that's odd. How do you build the tests?

I use "make -C tools/testing/selftests/resctrl" while in the root kernel
source directory. I tried to get the same error you experienced by
compiling some dummy test program with "open" and "O_WRONLY". From the
experiment I found that the "resctrl.h" header provides the declarations
that are causing your errors.

>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>> index bd36ee206602..b0b14a5bcbf5 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -488,9 +488,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
>>   */
>>  int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
>>  {
>> -	char controlgroup[1024], schema[1024], reason[64];
>> -	int resource_id, ret = 0;
>> -	FILE *fp;
>> +	char controlgroup[1024], schema[1024], reason[128];
>> +	int resource_id, fd, schema_len = -1, ret = 0;
>>  
>>  	if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) &&
>>  	    strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) &&
>> @@ -518,27 +517,30 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
>>  
>>  	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) ||
>>  	    !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
>> -		sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata);
>> +		schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n",
>> +				      "L3:", resource_id, '=', schemata);
>>  	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) ||
>>  	    !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)))
>> -		sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata);
>> +		schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n",
>> +				      "MB:", resource_id, '=', schemata);
>>  
>> -	fp = fopen(controlgroup, "w");
>> -	if (!fp) {
>> +	fd = open(controlgroup, O_WRONLY);
>> +	if (!fd) {
>>  		sprintf(reason, "Failed to open control group");
>
>It makes code easier to understand and maintain if it is kept
>consistent. It is thus unexpected for open() error handling to
>be untouched while write() error handling is modified. I think
>the addition of errno in error handling of write() is helpful. 
>Could you do the same for open()?

Okay, I'll add that, thanks.

-- 
Kind regards
Maciej Wieczór-Retman