On Mon, 29 Jun 2026 19:40:29 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). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Add warning for non-writable /tmp directory suggesting to use AltTempDir. A number of suggestions, but my main concern is about the actual length limit in the different cases. src/hotspot/os/linux/os_linux.cpp line 1554: > 1552: struct stat mystat; > 1553: int ret_val = stat(name, &mystat); > 1554: return (ret_val != -1 && S_ISDIR(mystat.st_mode) > 0 && access(name, > R_OK|W_OK|X_OK) == 0); Why do we need eXecute permission? src/hotspot/os/linux/os_linux.cpp line 1565: > 1563: // /proc/{vmid}/root/tmp/{PERFDATA_NAME_user}, otherwise > /tmp/{PERFDATA_NAME_user}, so we add 22. > 1564: > 1565: // We also check that it is a fully qualified pathname. Suggestion: // We need to check that any given alternate temporary directory name isn't too long, // is a writable directory, and specifies an absolute path. Revert back to hardcoded /tmp otherwise. // Since the attach mechanism uses the socket name length, this severely limits the length of the // alternate temporary directory name. // If in a containerized process, temp can be used to compose the dirname of // /proc/{vmid}/root/tmp/{PERFDATA_NAME_user}, otherwise /tmp/{PERFDATA_NAME_user}, so we allow // 22 characters for the prefix. const int CONTAINER_PREFIX_LEN = 22; src/hotspot/os/linux/os_linux.cpp line 1574: > 1572: if (AltTempDir != nullptr && AltTempDir[0] != '\0') { > 1573: if (AltTempDir[0] != '/') { > 1574: log_warning(os)("Warning: AltTempDir is ignored because it must > be a fully qualified pathname"); Suggestion: log_warning(os)("Warning: AltTempDir is ignored because it must be an absolute pathname"); I don't think "fully qualified" has any meaning for paths - they are absolute or relative. src/hotspot/os/linux/os_linux.cpp line 1577: > 1575: AltTempDir = nullptr; > 1576: } else { > 1577: size_t safe_max = UNIX_PATH_MAX - 22; // accounting for > /proc/%pid composition in containers Suggestion: size_t safe_max = UNIX_PATH_MAX - CONTAINER_PREFIX_LEN; src/hotspot/os/linux/os_linux.cpp line 1582: > 1580: AltTempDir = nullptr; > 1581: } else if (!is_writable_directory(AltTempDir)) { > 1582: log_warning(os)("Warning: AltTempDir is ignored because it is > not present or writable"); Suggestion: log_warning(os)("Warning: AltTempDir is ignored because it is not an existing, writable, directory"); src/hotspot/os/linux/os_linux.cpp line 1588: > 1586: } else { > 1587: if (!is_writable_directory("/tmp")) { > 1588: log_warning(os)("Warning: /tmp is not a writable directory. > Consider using -XX:AltTempDir=/<dir> to one that is writable"); Suggestion: log_warning(os)("Warning: /tmp is not writable. Consider using -XX:AltTempDir=/<dir> to set a writable temp directory"); src/hotspot/share/runtime/arguments.cpp line 1725: > 1723: > 1724: os::check_temp_directory(); > 1725: os::init_container_support(); Can't we place `check_temp_directory()` call inside the Linux `pd_init_container_support()` function and so avoid needing to add yet another method to the OS API (especially when it is actually only needed on one OS). test/jdk/com/sun/tools/attach/JvmTempDirTest.java line 84: > 82: runNoExistTest(noExist); > 83: > 84: Path veryLongDir = Files.createTempDirectory(Path.of("/tmp"), > "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); We need to test values around the UNIX_PATH_MAX-22 (or 20) length limit, not just one arbitrarily long one. test/jdk/com/sun/tools/attach/JvmTempDirTest.java line 100: > 98: * 1. Start the Application class in a separate process. > 99: * 2. Find the pid and shutdown port of the running Application. > 100: * 3. Launches the tests in nested class TestMain that will attach > to the Application. Suggestion: * 3. Launch the tests in nested class TestMain that will attach to the Application. test/jdk/com/sun/tools/attach/JvmTempDirTest.java line 209: > 207: > 208: private static void runLongTest(Path tmpDir) throws Throwable { > 209: Suggestion: private static void runLongTest(Path tmpDir) throws Throwable { test/jdk/com/sun/tools/attach/JvmTempDirTest.java line 219: > 217: > 218: private static void runNoExistTest(Path tmpDir) throws Throwable { > 219: Suggestion: private static void runNoExistTest(Path tmpDir) throws Throwable { test/jdk/com/sun/tools/attach/JvmTempDirTest.java line 229: > 227: > 228: private static void runRelativeTest(Path tmpDir) throws Throwable { > 229: Suggestion: private static void runRelativeTest(Path tmpDir) throws Throwable { ------------- Changes requested by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/31407#pullrequestreview-4595069582 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3494447737 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3494491285 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3494468399 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3494495748 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3494502467 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3494523743 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3494595828 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3494612111 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3494616472 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3494622696 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3494623541 PR Review Comment: https://git.openjdk.org/jdk/pull/31407#discussion_r3494624573
