Writing bitmasks to the schemata can fail when the bitmask doesn't
adhere to constraints defined by what a particular CPU supports.
Some example of constraints are max length or having contiguous bits.
The driver should properly return errors when any rule concerning
bitmask format is broken.
Resctrl FS returns error codes from fprintf() only when fclose() is
called. Current error checking scheme allows invalid bitmasks to be
written into schemata file and the selftest doesn't notice because the
fclose() error code isn't checked.
Substitute fopen(), flose() and fprintf() with open(), close() and
write() to avoid error code buffering between fprintf() and fclose().
Remove newline character from the schema string after writing it to
the schemata file so it prints correctly before function return.
Pass the string generated with strerror() to the "reason" buffer so
the error message is more verbose. Extend "reason" buffer so it can hold
longer messages.
Signed-off-by: Wieczor-Retman Maciej <[email protected]>
---
Changelog v3:
- Rename fp to fd (Ilpo)
- Remove strlen, strcspn and just use the snprintf value instead (Ilpo)
Changelog v2:
- Rewrite patch message.
- Double "reason" buffer size to fit longer error explanation.
- Redo file interactions with syscalls instead of stdio functions.
tools/testing/selftests/resctrl/resctrlfs.c | 26 +++++++++++----------
1 file changed, 14 insertions(+), 12 deletions(-)
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");
ret = -1;
goto out;
}
-
- if (fprintf(fp, "%s\n", schema) < 0) {
- sprintf(reason, "Failed to write schemata in control group");
- fclose(fp);
+ if (write(fd, schema, schema_len) < 0) {
+ snprintf(reason, sizeof(reason),
+ "write() failed : %s", strerror(errno));
+ close(fd);
ret = -1;
goto out;
}
- fclose(fp);
+ close(fd);
+ schema[schema_len - 1] = 0;
out:
ksft_print_msg("Write schema \"%s\" to resctrl FS%s%s\n",
--
2.42.0
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
Hi Maciej,
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 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.
Reinette
On 2023-09-12 at 09:00:28 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>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?
--
Kind regards
Maciej Wieczór-Retman
Hi Maciej,
On 9/12/2023 10:59 PM, Maciej Wieczór-Retman wrote:
> On 2023-09-12 at 09:00:28 -0700, Reinette Chatre wrote:
>> Hi Maciej,
>>
>> 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.
Reinette
Hi,
On 2023-09-13 at 11:49:19 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 9/12/2023 10:59 PM, Maciej Wieczór-Retman wrote:
>> On 2023-09-12 at 09:00:28 -0700, Reinette Chatre wrote:
>>> Hi Maciej,
>>>
>>> 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)?
--
Kind regards
Maciej Wieczór-Retman
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.
Reinette
[1] https://lore.kernel.org/lkml/[email protected]/
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/[email protected]/
>
--
Kind regards
Maciej Wieczór-Retman