On Thu, 12 May 2022 14:00:44 GMT, Thomas Stuefe <[email protected]> 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
