On Fri, 12 Dec 2025 12:44:09 GMT, Serguei Spitsyn <[email protected]> wrote:
>> 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. >> >>> ... > > @sercher I have a plan to submit mach5 tiers 1-6 to be completely safe from > regressions. > Will approve this PR if the results are good. Thank you @sspitsyn and @larry-cable for your reviews. ------------- PR Comment: https://git.openjdk.org/jdk/pull/25824#issuecomment-3655030577
