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

[Ioi Lam]

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

Changes requested by iklam (Reviewer).

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

> 91:                                               T returnval) {
> 92:   if (c == nullptr) {
> 93:     log_debug(os, container)("subsystem_file_line_contents: CgroupController* is nullptr");

The function name `subsystem_file_line_contents` is a bit vague. Maybe it should be changed to something like `parse_subsystem_file()`?

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

> 116:   }
> 117: 
> 118:   const int buf_len = MAXPATHLEN+1;

This is an existing bug. The contents of a line may theoretically be longer than MAXPATHLEN. It's possible to have a line like this:

somekey /some/file

Or it could just be some arbitrary values that have a very long string.

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

> 140:           && isspace(after_key) != 0
> 141:           && after_key != '\n') {
> 142:         // Skip key, skip space

When reading a string (scanning for `"%s"`), I am not sure if cgroup allows empty values for keys. If the input is like this:


char line1[] = "key ";
char line2[] = "key";


Then this function will return `OSCONTAINER_ERROR` for both cases. (This behavior is the same before and after this PR).

This might be OK (if cgroup will never have an empty value), but I think this should be documented to avoid surprises.

Also a test case is needed.

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

> 99:       const char* matchline = "hierarchical_memory_limit";
> 100:       GET_CONTAINER_INFO_LINE(julong, _memory->controller(), "/memory.stat", matchline,
> 101:                              "Hierarchical Memory Limit is: " JULONG_FORMAT, JULONG_FORMAT, hier_memlimit)

For consistency, I think the "matchline" variable should be renamed to 'key', or just passed as a literal string in the GET_CONTAINER_INFO_LINE call.

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

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