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:   Fri, 15 Sep 2023 10:16:56 +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: <3a24bhplwgpn343hc4vwze2jnt4jebzvoigtbzom4gb5xuyb4c@ffs2qhy3ouay>
References: <cover.1693575451.git.maciej.wieczor-retman@intel.com>
 <960b5302cee8e5bb0e83dcf20cd2ef4d353b7b0b.1693575451.git.maciej.wieczor-retman@intel.com>
 <99eabc5c-f5e3-27dd-0a29-ad0cdb7b4239@intel.com>
 <sr6ana6d7ebtuxbhjuo6kcnhnn2zzvg3ivve6mndqeb3nxrzo7@mrfmtzlxlwdh>
 <a7f0dfb9-c841-b240-fca9-b908517a44d0@intel.com>
 <jyxp5mspjn7xbmclj5sumbsuwd424fqmdvntiiuq24tiz5yqb4@qunho7whbamf>
 <6d6dba28-a7f0-1f49-3f59-4e0026d0c2b8@intel.com>
 <ycbcan5wcxtz2mbywz6zdoyfus26vrhus6enipe543x2ejyt77@m3l4ksir7k2k>
 <6fbc0b65-0f02-3c7f-bd7a-5183d24a1fe5@intel.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <6fbc0b65-0f02-3c7f-bd7a-5183d24a1fe5@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?L2JrUnZlbEhyZE0vMGhjUkxMVHdNS3pKOEtibXFwczcxUWdCNTVqYmd1ZU9T?=
 =?utf-8?B?OGRXMW81cndmZTl4anBFcDVNS0psUFk3KytYWTU2OVRrTlluQ2p1ZTBxMGov?=
 =?utf-8?B?Q2F4VDRFN2prOVkyZWlnYmRwbWlvWTFYbnp5aTF5bnJNVnFOYzVzbFNySmtR?=
 =?utf-8?B?RXY4M1pxRnV2cmRJSjJxT0wxZUZieVBzSUhmTUpLUGJ2UjZ4RG1vaHowSG5q?=
 =?utf-8?B?L2paRG5CVkVtNVRwTHBvN2FDZ3hzaEo1c0ZtN1JlMVJJT29hOWNjRktKcEc2?=
 =?utf-8?B?aVMrTkE4VDFvQWFvMy9RNzRQTUlLc0pnMkp2aHd1VnFyTjJkQ1MvcGJXYWt4?=
 =?utf-8?B?Nmcxd1RYNFhoK05zV0E5V21RdjBNQTJtc21RRmRqZzdFdXU4OTgzSU12b1Nn?=
 =?utf-8?B?dTIvQkFpSFBOOTNhSkt0UWIwdjlQNS9ySmVQWUxOMUtwVnFwUkNQcVhuRVRQ?=
 =?utf-8?B?cFRGY1JTVk9IOHRZSUR5eUQ2clVWUzd5V0kvQWN6WXVFK1lubC96NUU0aUsv?=
 =?utf-8?B?ejE1bTJZU0hlVG43K1I2WnIxV1VkeDBmSDdVN1RQUTlUZzgraG91ZytlaHlZ?=
 =?utf-8?B?NVlTZTFmUXlPZjFnOXhaSXJFVVI3azh4amJJRjA5NkJZYzRCM3E2a2lXQzRH?=
 =?utf-8?B?YTlMdFQ4Y0RuY0IzU1NVaG9SbkIvcjREN3ZJSjUybGpKcCt6cEFWV2lDcGg4?=
 =?utf-8?B?VW5xamFCSUg1cm91VXhOOVRDUHlLc0RIZ21DNzJEazRBQW81VVVOUmQyZkg0?=
 =?utf-8?B?bndXcmt0NHJPSVd5djIvaTlDT0YxMko0bFA5M2RRellmdkdMb3RPYzVsa3F1?=
 =?utf-8?B?d04vK3lUZ0ZQRE50VXo1ZlpCalE4bTFHOE93ZU5qaTNySGRGbnVqYnV0eldv?=
 =?utf-8?B?aTRqSllXRDZJQzJvdEVad0VvekF5ZExVM2tTOTlGbXdlc0JRY0t1U083UUla?=
 =?utf-8?B?dE5BeUN3RlJ2VDFuUS9XVHhHcWJ5ajJHZ25WRWJGL0JSeHFmK3BYN0FqWHg1?=
 =?utf-8?B?S1dZZUpDUzcxb0FGY0FnN05LTCsvWmFaSktzMTh3TkVLK25SRHVsVXdmeWcv?=
 =?utf-8?B?OWNBclE1LzVnRW9idElxWXBNeDlmM2ZvTnp5cDh1TDVZejlmNTQ3NmFZZTRL?=
 =?utf-8?B?cGtJLzFUN3VhZ3ZtQlJsSU1nLzBaWndIOTZnMGozNHIxRHMxY0xiNkxKSnY1?=
 =?utf-8?B?Yjg3V0l3b3FaVGc2Sy9KbWVQVVNzemE0ZVllZ1N6YXEwSTRTbGpveDFRSjhu?=
 =?utf-8?B?VjQ4TGptSnljQVMwUlJaa0JFUWRTaWs3WnhNZ0VyeVNMUDE0aTI1Zlc3Vy9p?=
 =?utf-8?B?Q1puS1VsUS9nQW1jM3J0QzlqWkl5eXY0MXF6NStBenRiMmdHL3FJYzVxZFpZ?=
 =?utf-8?B?dHlMOUl1R3h4TXF5MGpTRkdhd0RvZjBYMlpHVXF5NWF3ZW5hZDBBMjRIeS9o?=
 =?utf-8?B?KzZjSXhYWUZpdGNqTUpTaThKa0w0c1Robkh1YkxQdHlrT2JWa2Z4MEUvSWgr?=
 =?utf-8?B?amRJSzM5b25pbGFhVkxKUElrbDFPdy93dlE1Y004OVFNeHZKQ2tQMHhuM1g5?=
 =?utf-8?B?bk1QZmtLQTBRclkyZ3YvcTlnNktjQmU5enB5RmJuSXhORVpROURXRndUTGRh?=
 =?utf-8?B?S3dITVdVZHN5OUFPRjFNYUtmQ3JKV1VrZis1S1U4azVHNktJRzZwZzFKVmlt?=
 =?utf-8?B?M0k3UkoyeDN3cTFrajY5eHYrQmw1a0FSVHQvMVEyWDhSNDFQRmtGN3pzQW1o?=
 =?utf-8?B?N2N6V25NK1RmUG9YMmJrZW5pZE9KY2haUHRrNGE2ZnBuL0VZTW9Ic3NsZ29n?=
 =?utf-8?B?UGFmRnd1ek9ieUxabXV6Q2xjamEvMlA1SUQzK052VkorNmdCQUI0Vmx0Zlh3?=
 =?utf-8?B?VWlacm93eVBXQVBiYVBIa0dqenh6NWx2VXVuc2UxNVhkSzRISFJyYmhCWW9n?=
 =?utf-8?B?bVNENk14a3F0RnFaWU01bHhIcXVIbUkwL3hyMXk0S2ZtbThWNHZSVVlSMDY5?=
 =?utf-8?B?Zk9IUFFWMzBWTS8xNmFHOGhZaytMNDdVWDFtZ1duMi9JN1JJRm94QWtSTU9p?=
 =?utf-8?B?eGlVcUIwMDZvcjltS01vaE9mUS9xTnNBZzR3V3ZlU2d2dzRQMG9UVjRTTlha?=
 =?utf-8?B?RTI2aWxJRXdqbGFRMTVraUVPd1JZVjdIM1FjSWQrODEzeDk3K2wrVTJ4b3hu?=
 =?utf-8?Q?mLiDpIuj9Kq8B4W7pJNTwJ0=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b3862636-e134-4af9-903e-08dbb5c42403
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6231.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Sep 2023 08:17:02.6895
 (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: 5HvVyaBOrCQ+5GsCCpoy74hpwLVH9ZIA1RBXwrGQeqcJtvTHqPf/MhetnxkkLO2wmG+iI1M9GguaNgzHhkyWVdl++kxCzMKJrhoxLo1zzOE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5225
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 autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.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 (pete.vger.email [0.0.0.0]); Fri, 15 Sep 2023 01:29:48 -0700 (PDT)

On 2023-09-14 at 08:14:25 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 9/13/2023 11:01 PM, Maciej Wieczór-Retman wrote:
>> On 2023-09-13 at 11:49:19 -0700, Reinette Chatre wrote:
>>> On 9/12/2023 10:59 PM, Maciej Wieczór-Retman wrote:
>>>> On 2023-09-12 at 09:00:28 -0700, Reinette Chatre wrote:
>>>>> On 9/11/2023 11:32 PM, Maciej Wieczór-Retman wrote:
>>>>>> 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 applied this series on top of kselftest repo's "next" branch.
>>>>>
>>>>> I use a separate build directory and first ran "make headers". After that,
>>>>> $ make O=<build dir> -C tools/testing/selftests/resctrl
>>>>
>>>> I do the same, just without the build directory, but that shouldn't
>>>> matter here I guess.
>>>>
>>>>>> 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.
>>>>>
>>>> >From what I can tell resctrl.h does not include fcntl.h that provides
>>>>> what is needed.
>>>>
>>>> I found out you can run "gcc -M <file>" and it will recursively tell you
>>>> what headers are including other headers.
>>>>
>>>> Using this I found that "resctrl.h" includes <sys/mount.h> which in turn
>>>> includes <fcntl.h> out of /usr/include/sys directory. Is that also the
>>>> case on your system?
>>>>
>>>
>>> No. The test system I used is running glibc 2.35 and it seems that including
>>> fcntl.h was added to sys/mount.h in 2.36. See glibc commit
>>> 78a408ee7ba0 ("linux: Add open_tree")
>>>
>>> Generally we should avoid indirect inclusions and here I think certainly so
>>> since it cannot be guaranteed that fcntl.h would be available via 
>>> sys/mount.h.
>> 
>> Okay, would including the fcntl.h header to resctrl.h be okay in this
>> case? Or is there some other more sophisticated way of doing that (some
>> include guard or checking glibc version for example)?
>
>Ideally fcntl.h would be included in the file it is used. Doing so you may
>encounter the same problems as Ilpo in [1]. If that is the case and that fix works
>for you then you may want to have this series depend on Ilpo's work.

Thanks a lot for finding this, and yes, I get the same errors by adding the
header. I'll send the next version of this series with the added header
rebased on top of Ilpo's series you mentioned.

>Reinette
>
>[1] https://lore.kernel.org/lkml/dfc53e-3f92-82e4-6af-d1a28e8c199a@linux.intel.com/
> 

-- 
Kind regards
Maciej Wieczór-Retman