RFR: 8287007: [cgroups] Consistently use stringStream throughout parsing code [v2]

Ioi Lam iklam at openjdk.java.net
Wed Jun 8 07:21:34 UTC 2022


On Tue, 7 Jun 2022 12:42:26 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> Please review this cleanup change in the cgroup subsystem which used to use hard-coded stack allocated
>> buffers for concatenating strings in memory. We can use `stringStream` instead which doesn't have the issue
>> of hard-coding maximum lengths (and related checks) and makes the code, thus, easier to read.
>> 
>> While at it, I've written basic `gtests` verifying current behaviour (and the same for the JDK code). From
>> a functionality point of view this is a no-op (modulo the one bug in the substring case which seems to be
>> there since day one). I'd prefer if we could defer any change in functionality to a separate bug as this is
>> meant to only use stringStream throughout the cgroups code.
>> 
>> Testing:
>> - [x] Container tests on Linux x86_64 cgroups v1 and cgroups v2
>> - [x] Added tests, which I've verified also pass before the stringStream change
>> 
>> Thoughts?
>
> Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Merge branch 'master' into jdk-8287007-string-stream
>  - Add cgroups v2 java test
>  - use stringStream in cgroups v2
>  - Add gtest for cgroups v2 code path
>    
>    Also fixes the bug when cgroup path is '/'.
>  - 8287007: [cgroups] Consistently use stringStream throughout parsing code
>  - 8287007: [cgroups] Consistently use stringStream throughout parsing code

Changes requested by iklam (Reviewer).

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

> 52:       } else {
> 53:         char *p = strstr(cgroup_path, _root);
> 54:         if (p != NULL && p == cgroup_path) {

I think this change should be done in a separate bug, because it will cause the `if` block to be executed. Previously the `if` block is never executed (unless `cgroup_path == _root` ??, but then it will just set `_path` to the same string as `_mount_point`) -- so we don't even know if this change of behavior might do something harmful.

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

PR: https://git.openjdk.java.net/jdk/pull/8969


More information about the serviceability-dev mailing list