RFR: 8298730: Refactor subsystem_file_line_contents and add docs and tests

[Severin Gehwolf]

On Wed, 14 Dec 2022 10:18: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"`.

Some initial comments. I'll run my container testing on it and will let you know.

src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 139:

> 137:       char* key_substr = strstr(line, key);
> 138:       // key should be at beginning of line and next character space
> 139:       if (key_substr == line && line[key_len] == ' ') {

This now expects files to be space (` `) separated. I'm not sure we can be sure of it. How about using `isspace(line[key_len]) != 0` instead?

src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 154:

> 152:     return 0;
> 153:   }
> 154:   log_debug(os, container)("Type %s not found in file %s", scan_fmt, absolute_path);

Previously the log would include more information as the `scan_fmt` now only includes the format of the value to read in. Perhaps we should changes this to the following for better diagnostics:


log_debug(os, container)("Type %s (key == %s) not found in file %s", scan_fmt, (key == nullptr ?  "null" : key), absolute_path);

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 287:

> 285: char* CgroupV1Subsystem::pids_max_val() {
> 286:   GET_CONTAINER_INFO_CPTR(cptr, _pids, "/pids.max",
> 287:                      "Maximum number of tasks is: %s", "%s", pidsmax, 1024);

To keep the format specifiers in sync, please use `%1023s` over `%s` here instead.

src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 115:

> 113:   return os::strdup(mems);
> 114: }
> 115: 

This seems a no-op change, please remove from the patch.

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

> 79: }
> 80: 
> 81: TEST(CgroupTest, SubSystemFileLineContentsTests) {

`test/hotspot/gtest/runtime/test_os_linux_cgroups.cpp` uses tag `os_linux_cgroup` and underscore-separated (over camelCase) name. Could we make this consistent? Ideally, all cgroup related gtests should get selected with something like `gtest:os_linux_cgroup` or `gtest:cgroup` or a better name you come up with.

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

> 135:   err = subsystem_file_line_contents(&my_controller, test_file, "foo", "%s", &s);
> 136:   EXPECT_EQ(err, 0);
> 137:   EXPECT_STREQ(s, "1");

We should have a test where the separator isn't a single space, like `\t` instead. See https://github.com/jerboaa/jdk/commit/8ab1009a18a95510504f70d8a02c822083fdffd1 That's when those tests currently fail. Like I said previously it would also be good to have a test similar to how `CgroupV2Subsystem::cpu_period()` uses it. Like 
https://github.com/jerboaa/jdk/commit/4d562f08952e17f9137c0da74a3bb587c8539713

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

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