On Thu, 4 Dec 2025 00:31:37 GMT, Serguei Spitsyn <[email protected]> wrote:
>> Sergey Chernyshev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> addressed review comments > > We had a private discussion with Larry on this PR and undesired dependency on > the `sun.jvmstat` module. > We got the following concerns: > - Do we really need 3x implementations of the MacOS `tempdir` finder > function? > - Do we really need a modular dependency from `jdk.attach` to > `jdk.internal.jvmstat`? > New method `getTemporaryDirectories(int pid)` is placed into the > `jdk.internal.jvmstat/macosx/classes/sun/jvmstat/PlatformSupportImpl.java`. > It seems it was needed to inherit the `PlatformSupport` class which has the > method `PlatformSupport.getTemporaryDirectory()`. But it returns the same as > the removed function `Java_sun_tools_attach_VirtualMachineImpl_getTempDir()`. > So, this implementation can be moved to the `jdk.attach` module. > My understanding is that class `PlatformSupportImpl` and its method > `getTemporaryDirectories(int pid)` is not currently used for `jvmstat` > implementation. Is it correct? Is it right that the `jvmstat` implementation > still does not properly support root user on macOS? > Anyway, some unification and refactoring is needed here. We could file a > follow up enhancement to sort this out but also wanted to know your opinion > on this. Hi @sspitsyn , Thank you for looking at this patch! > This is a nice fix in general. Thank you for this work! I hope to complete my > review tomorrow with one more pass. Question: How was this update tested? Do > you have a jtreg test or you've done it manually? Also, did you run mach5 > tiers 1-6 to be safe and protected against regressions? The patch was tested against the current jtreg test groups (tier1-3) on macOS both amd64 and arm64. I also run jcmd and jps tools manually under root, because the change only affects the behavior under root. > * It seems it was needed to inherit the `PlatformSupport` class which has the > method `PlatformSupport.getTemporaryDirectory()`. But it returns the same as > the removed function `Java_sun_tools_attach_VirtualMachineImpl_getTempDir()`. > So, this implementation can be moved to the `jdk.attach` module. The removed function `Java_sun_tools_attach_VirtualMachineImpl_getTempDir()` would return always the same string cached inside the libsystem_coreservices.dylib, that would correspond the user specific temporary path as of how the JVM process has started, that would have nothing to do with the target process/pid temporary directory. > * My understanding is that class `PlatformSupportImpl` and its method > `getTemporaryDirectories(int pid)` is not currently used for `jvmstat` > implementation. Is it correct? I think this is not exactly so. `getTemporaryDirectories(int pid)` is used currently in the LocalVmManager to get the list of active VMs, that is consumed by `jcmd`, `jps` and `jconsole` utilities. When 0 is passed as a parameter, it returns the list of temp directories for all processes, if they were different (for example if there are containerized JVM processes under Linux). > * Is it right that the `jvmstat` implementation still does not properly > support root user on macOS? Yes and this is what this patch was intended for. > * Do we really need 3x implementations of the MacOS `tempdir` finder > function? It was already 2x implementation of HotSpot perfdata files lookup, HotSpot itself uses the `os::get_temp_directory()`, on the java side the LocalVmManager in `sun.jvmstat.perfdata.monitor` relies on `getTemporaryDirectories(int pid)` in the PlatformSupport. The 3rd was the macOS native `Java_sun_tools_attach_VirtualMachineImpl_getTempDir()`, now it's proposed for removal as it returns only the current user temp path, no matter what process is being attached to. > * Do we really need a modular dependency from `jdk.attach` to > `jdk.internal.jvmstat`? It was already a dependency of `jdk.attach` to `sun.jvmstat.monitor`. This change exports also `sun.jvmstat`, in addition to `sun.jvmstat.monitor` as it was before. > src/hotspot/os/bsd/os_bsd.cpp line 956: > >> 954: } >> 955: #endif >> 956: > > Nit: I'd suggest to get rid of empty lines which do not improve readability: > 890, 903, 908, 917, 923, 930, 933, 941, 944 > Also, the comment at line 931 is better to start with a capital letter: `// > If the ..` . Thanks @sspitsyn , done. ------------- PR Comment: https://git.openjdk.org/jdk/pull/25824#issuecomment-3616052858 PR Review Comment: https://git.openjdk.org/jdk/pull/25824#discussion_r2592160722
