Re: RFR: 8287007: [cgroups] Consistently use stringStream throughout parsing code [v2]
On Wed, 8 Jun 2022 08:25:21 GMT, Severin Gehwolf wrote:
>> 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.
>
> OK. Then this will remain to be dead code and I'll remove the corresponding
> test case in the new `gtests` too (as they'd otherwise fail in contrast to
> the Java code).
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/8969
Re: RFR: 8287007: [cgroups] Consistently use stringStream throughout parsing code [v2]
On Wed, 8 Jun 2022 07:13:30 GMT, Ioi Lam wrote:
>> 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
>
> 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.
OK. Then this will remain to be dead code and I'll remove the corresponding
test case in the new `gtests` too (as they'd otherwise fail in contrast to the
Java code).
-
PR: https://git.openjdk.java.net/jdk/pull/8969
Re: RFR: 8287007: [cgroups] Consistently use stringStream throughout parsing code [v2]
On Tue, 7 Jun 2022 12:42:26 GMT, Severin Gehwolf 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
Re: RFR: 8287007: [cgroups] Consistently use stringStream throughout parsing code [v2]
> 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: - all: https://git.openjdk.java.net/jdk/pull/8969/files - new: https://git.openjdk.java.net/jdk/pull/8969/files/a7b156a4..8047fe37 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8969=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8969=00-01 Stats: 60055 lines in 839 files changed: 34126 ins; 19620 del; 6309 mod Patch: https://git.openjdk.java.net/jdk/pull/8969.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8969/head:pull/8969 PR: https://git.openjdk.java.net/jdk/pull/8969
