Re: RFR: 8287007: [cgroups] Consistently use stringStream throughout parsing code [v3]
On Wed, 8 Jun 2022 12:09:27 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 incrementally with one > additional commit since the last revision: > > Remove fix for substring match @erikj79 Do you know why the SKARA bots didn't move this to `ready` by now? Is there an issue with the bots? Thanks! - PR: https://git.openjdk.org/jdk/pull/8969
Re: RFR: 8287007: [cgroups] Consistently use stringStream throughout parsing code [v3]
On Wed, 8 Jun 2022 12:09:27 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 incrementally with one > additional commit since the last revision: > > Remove fix for substring match Just came back from vacation. LGTM. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.org/jdk/pull/8969
Re: RFR: 8287007: [cgroups] Consistently use stringStream throughout parsing code [v3]
> 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 incrementally with one additional commit since the last revision: Remove fix for substring match - Changes: - all: https://git.openjdk.java.net/jdk/pull/8969/files - new: https://git.openjdk.java.net/jdk/pull/8969/files/8047fe37..84d952b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8969=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8969=01-02 Stats: 10 lines in 2 files changed: 0 ins; 7 del; 3 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
