RFR: 8302744: Refactor Hotspot container detection code [v4]

[Thomas Stuefe]

On Mon, 27 May 2024 19:54:16 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> Please review this container detection code refactoring in hotspot. The main point of this is to
>> 
>> - get rid of the `GET_CONTAINER_INFO` macros which hide too many things under the hood
>> - prevent refactoring of the code (since `GET_CONTAINER_INFO` macros short-return and are therefore not portable; at least not without some risk)
>> - make the code easier to understand
>> - allow for better testing via `gtest`
>> - separate multi-line parsing from single line parsing for clarity.
>> 
>> Testing:
>> - [x] GHA
>> - [x] `gtest:cgroupTest` tests
>> - [x] Container tests on Linux with cgroup v1 (legacy) and cgroup v2. All pass.
>> 
>> Thoughts?
>
> Severin Gehwolf has updated the pull request incrementally with four additional commits since the last revision:
> 
>  - Make read_string() take the output buffer size
>  - Fix style for read_numerical_key_value
>  - Use assertions for required parameters
>  - Use boolean for tuples

Final look at the gtests, small nits

test/hotspot/gtest/runtime/test_cgroupSubsystem_linux.cpp line 71:

> 69: class TestController : public CgroupController {
> 70: private:
> 71:   char* _path;

Make it const char* const, and init via initializer list in constructor

test/hotspot/gtest/runtime/test_cgroupSubsystem_linux.cpp line 76:

> 74:     _path = p;
> 75:   }
> 76:   char* subsystem_path() override {

Should be `const char* subsystem_path() const {..}`. Same for the virtual function in CgroupController.

test/hotspot/gtest/runtime/test_cgroupSubsystem_linux.cpp line 107:

> 105:   fill_file(test_file, "foo ");
> 106:   bool is_ok = controller->read_numerical_key_value(base_with_slash, "foo", &x);
> 107:   EXPECT_FALSE(is_ok) << "Value must not be missing in key/value case";

The output is confusing. The value is missing, right?

TBH, in many of these cases you could just omit the `<< "text"` explanations, since the EXPECT macros are already clear enough.

test/hotspot/gtest/runtime/test_cgroupSubsystem_linux.cpp line 133:

> 131:   EXPECT_FALSE(is_ok) << "key must be exact match";
> 132:   EXPECT_EQ((julong)0xBAD, x) << "x must be unchanged";
> 133: 

I would consider defining a `constexpr julong bad = 0xBAD`, then use that constant to initialize x and in all the `EXPECT_EQ` to get rid of the casts.

test/hotspot/gtest/runtime/test_cgroupSubsystem_linux.cpp line 153:

> 151:   bool is_ok = controller->read_numerical_key_value(base_with_slash, "foo", &x);
> 152:   EXPECT_TRUE(is_ok);
> 153:   EXPECT_EQ((julong)100, x) << "Incorrect!";

Here especially, the "Incorrect" messages seem pointless :)

test/hotspot/gtest/runtime/test_cgroupSubsystem_linux.cpp line 189:

> 187: }
> 188: 
> 189: TEST(cgroupTest, read_number_null) {

I think the subsystem==null case could maybe just be asserted, and this test then can be removed

test/hotspot/gtest/runtime/test_cgroupSubsystem_linux.cpp line 314:

> 312:   ok = controller->read_string(base_with_slash, result, 1024);
> 313:   EXPECT_TRUE(ok) << "String parsing should have been successful";
> 314:   EXPECT_STREQ("abc def", result) << "Expected strings to be equal";

These two cases, I think, could be removed, since read_string only does a plain fgets, so there should nothing exciting happening depending on the string content. 

If you want to be diligent, you could test that the trailing newline is trimmed off, and that leading/trailing spaces are preserved, e.g. by testing `" \n"` must be read as `" "`

test/hotspot/gtest/runtime/test_cgroupSubsystem_linux.cpp line 337:

> 335:   }
> 336:   result[0] = '\0';
> 337:   fill_file(test_file, too_large);

I would zero-terminate `too_large` before writing it to the file. `fill_file` expects a zero-terminated string as second arg

test/hotspot/gtest/runtime/test_cgroupSubsystem_linux.cpp line 343:

> 341:   for (int i = 0; i < 1023; i++) {
> 342:     EXPECT_EQ(too_large[i], result[i]) << "Expected item at idx " << i << " to match";
> 343:   }

Hmm, so the contract is that we don't report truncation in this case? That is a bit questionable. But probably for another RFE, if at all.

In any case, a simple `EXPECT_EQ(strncmp(too_large, result, 1023), 0)` can replace the comparison loop.

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

PR Review: https://git.openjdk.org/jdk/pull/19060#pullrequestreview-2081709939
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1616563056
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1616562848
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1616566047
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1616568460
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1616572135
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1616573803
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1616577242
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1616578758
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1616581871