RFR: 8298730: Refactor subsystem_file_line_contents and add docs and tests [v3]

Severin Gehwolf sgehwolf at openjdk.org
Thu Dec 15 09:48:08 UTC 2022


On Wed, 14 Dec 2022 19:28:32 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Hi!
>> 
>> Citing the ticket directly:
>> 
>> The function subsystem_file_line_contents does the simple job of parsing lines in a file, but does so in a fairly complex way. This function can be simplified. The function also has a surprising API, as you need to know that in one case the format string needs to take 2 specifiers, instead of 1. 
>> 
>> Some more context:
>> 
>> `subsystem_file_line_contents` either parses files that look like this:
>> 
>> 
>> one_value
>> 
>> 
>> Called as: `subsystem_file_line_contents(ctrl, fname, nullptr, "%s")`
>> 
>> Or like this:
>> 
>> 
>> key1 val1
>> key2 val2
>> 
>> 
>> Called as:  `subsystem_file_line_contents(ctrl, fname, "key1", "%s %s")`
>> 
>> The API for the key/value case is changed to: `subsystem_file_line_contents(ctrl, fname, "key1", "%s")`. Note: `"%s"`, not `"%s %s"`.
>
> Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add two more test cases

Looks fine, thanks!

test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp line 100:

> 98: 
> 99:   s[0] = '\0';
> 100:   fill_file(test_file, "foo\tbar");

Since this is in the multi-line test (`SubSystemFileLineContentsMultipleLines`), perhaps actually make it multi-line? Something like `fill_file(test_file, "foo\tbar\nfoot car");`

test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp line 143:

> 141:   err = subsystem_file_line_contents(&my_controller, test_file, nullptr, "%*s %d", &x);
> 142:   EXPECT_EQ(err, 0);
> 143:   EXPECT_EQ(x, 10001);

These are single-line tests, perhaps move them to `SubSystemFileLineContentsSingleLine`?

-------------

Marked as reviewed by sgehwolf (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11667


More information about the hotspot-runtime-dev mailing list