On Thu, 12 May 2022 14:00:44 GMT, Thomas Stuefe <stu...@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 > > src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 113: > >> 111: } >> 112: buf[MAXPATHLEN-1] = '\0'; >> 113: _path = os::strdup(buf); > > I think this code can be simplified a lot with stringStream and without > strtok, so no need for fixed buffers (which may fail with longer path names) > and no need for writable string copies on the stack. > > Something like this: > > stringStream ss; > ss.print_raw(_mount_point); > const char* p1 = _root; > const char* p2 = cgroup_path; > int last_matching_dash_pos = -1; > for (int i = 0; *p1 == *p2 && *p1 != 0; i ++) { > if (*p1 == '/') { > last_matching_dash_pos = i; > } > p1++; p2++; > } > ss.print_raw(_root, last_matching_dash_pos); > // Now use ss.base() to access the assembled string Nice, thanks! I'll update it. ------------- PR: https://git.openjdk.java.net/jdk/pull/8629