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

[Severin Gehwolf]

On Tue, 28 May 2024 04:25:09 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> 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
>
> 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.

I'm not sure this is possible. The subsystem path is not set by the constructor. I'll leave that for another day.

> 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.

Fixed.

> 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.

OK.

> 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

The subsystem path is being set dynamically (possibly null), so the null check should stay.

> 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 `" "`

Those two cases where there earlier and are valid positive checks (even if trivial). They returned `abc` for the space case prior this refactoring. Keeping the test shows this has been considered.

I've added the space with new-line case.

> 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

Fixed.

> 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.

Used `strncmp()` here.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1617162956
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1617161695
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1617166569
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1617168506
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1617171083
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1617171602
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1617172391