On Fri, 5 Jun 2026 18:11:24 GMT, Coleen Phillimore <[email protected]> wrote:
> Please review this change to allow XX configuration for specifying a > different /tmp directory for the JVM to use. In some container environments, > /tmp and /proc/pid/root/tmp might not be usable and an alternate would be > used. This requires a release note and CSR. Usage is: > > java -XX:AltTempDir=/diags <app> > jps -J-XX:AltTempDir=/diags > jcmd -J-XX:AltTempDir=/diags <pid> <cmds> > > Tested with a couple of tests and locally, and ran tier1-4. > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Overall looks good. A few minor nits. src/hotspot/os/linux/os_linux.cpp line 1560: > 1558: void os::pd_check_temp_directory() { > 1559: if (AltTempDir != nullptr && AltTempDir[0] != '\0') { > 1560: size_t safe_max = PATH_MAX - 100; // accounting for /proc/%pid > composition in containers PATH_MAX or UNIX_PATH_MAX? src/hotspot/os/linux/os_linux.cpp line 1562: > 1560: size_t safe_max = PATH_MAX - 100; // accounting for /proc/%pid > composition in containers > 1561: if (strlen(AltTempDir) > safe_max) { > 1562: log_warning(os)("Error: AltTempDir is ignored because it's longer > than %zd bytes", safe_max); It is logged as a warning but states it is an Error - these need to agree. src/hotspot/os/linux/os_linux.cpp line 1565: > 1563: AltTempDir = nullptr; > 1564: } else if (!is_writeable_directory(AltTempDir)) { > 1565: log_warning(os)("Error: AltTempDir is ignored because it is not > present or writable"); Ditto: warning or error? src/hotspot/os/linux/os_linux.cpp line 1565: > 1563: AltTempDir = nullptr; > 1564: } else if (!is_writeable_directory(AltTempDir)) { > 1565: log_warning(os)("Error: AltTempDir is ignored because it is not > present or writable"); Suggestion: log_warning(os)("Error: AltTempDir is ignored because it is not an existing writeable directory"); src/hotspot/os/linux/os_linux.cpp line 1568: > 1566: AltTempDir = nullptr; > 1567: } > 1568: } Suggestion: } else { AltTempDir = nullptr; } That way we don't need to check AltTempDir[0] again at line 1573 - if it is non-null then it is valid. src/hotspot/os/posix/attachListener_posix.cpp line 349: > 347: int ret; > 348: > 349: int n = os::snprintf(fn, UNIX_PATH_MAX, "%s/.java_pid%d", UNIX_PATH_MAX or PATH_MAX? src/hotspot/os/posix/attachListener_posix.cpp line 354: > 352: log_warning(attach)("Failed to attach using temporary file %s", fn); > 353: return; > 354: } Is runtime truncation actually possible? src/hotspot/os/posix/perfMemory_posix.cpp line 149: > 147: if (nspid != -1) { > 148: int val = jio_snprintf(buffer, PATH_MAX, "/proc/%d/root%s", vmid, > tmpdir); > 149: assert(val != -1, "should not truncate, because length was already > checked"); Suggestion: assert(val != -1, "should not truncate, because tmpdir length was already limited"); src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java line 261: > 259: private String findTargetProcessTmpDirectory(long pid) throws > IOException { > 260: final var tmpOnProcPidRoot = > PROC.resolve(Long.toString(pid)).resolve("root") > 261: .resolve(vmTemp.startsWith("/") ? > vmTemp.substring(1) : vmTemp); Suggestion: final var tmpOnProcPidRoot = PROC.resolve(Long.toString(pid)).resolve("root") .resolve(vmTemp.startsWith("/") ? vmTemp.substring(1) : vmTemp); test/jdk/com/sun/tools/attach/TempDirTest.java line 69: > 67: // Run the test with all possible combinations of setting > java.io.tmpdir. > 68: // Note that the attach mechanism doesn't really use > java.io.tmpdir, but this test verifies > 69: // that different java.io.tmpdir settings for client and target > doesn't break the attach mechanism. Suggestion: // that different java.io.tmpdir settings for client and target don't break the attach mechanism. ------------- PR Review: https://git.openjdk.org/jdk/pull/31407#pullrequestreview-4455708192 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3378297166 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3378266100 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3378270495 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3378276673 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3378292020 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3378299562 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3378308086 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3378394429 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3378421126 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3378579571
