On Tue, 10 May 2022 12:29:10 GMT, Severin Gehwolf <sgehw...@openjdk.org> wrote:

> Please review this change to the cgroup v1 subsystem which makes it more 
> resilient on some of the stranger systems. Unfortunately, I wasn't able to 
> re-create a similar system as the reporter. The idea of using the longest 
> substring match for the cgroupv1 file paths was based on the fact that on 
> systemd systems processes run in separate scopes and the maven forked test 
> runner might exhibit this property. For that it makes sense to use the common 
> ancestor path. Nothing changes in the common cases where the `cgroup_path` 
> matches `_root` and when the `_root` is `/` (container the former, host 
> system the latter).
> In addition, the code paths which are susceptible to throw NPE have been 
> hardened to catch those situations. Should it happen in the future it makes 
> more sense (to me) to not have accurate container detection in favor of 
> continuing to keep running.
> Finally, with the added unit-tests a bug was uncovered on the "substring" 
> match case of cgroup paths in hotspot. `p` returned from `strstr` can never 
> point to `_root` as it's used as the "needle" to find in "haystack" 
> `cgroup_path` (not the other way round).
> Testing:
> - [x] Added unit tests
> - [x] GHA
> - [x] Container tests on cgroups v1 Linux. Continue to pass

I just started to look at the code so just one comment for now.

 line 65:

> 63:                             path = mountPoint + cgroupSubstr;
> 64:                         }
> 65:                     } else {

Looks like `path` is still not set if the condition at line 61 `if 
(cgroupPath.length() > root.length()) {` is false.


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

Reply via email to