RFR: 8271949: dumppath in -XX:FlightRecorderOptions does not affect [v3]

[Yasumasa Suenaga]

On Fri, 3 Sep 2021 15:23:54 GMT, Markus Grönlund <mgronlun at openjdk.org> wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update testcase
>
> Hi Yasumasa,
> 
> I noted the following when stepping through the patch:
> 
> 1. Mismatch in default behaviour for paths between option "repository" and "dumppath" for relative paths:
> 
> -XX:FlightRecorderOptions=repository=apa,dumppath=ko
> 
> Repository path: $PWD/apa
> Emergency dump path: ko
> 
> 2. Modified default output in hs_err_pid.log file:
> 
> From:
> <pre>
> # CreateCoredumpOnCrash turned off, no core file dumped"
> #
> # JFR recording file will be written. Location: D:\utilities\jtreg\runtime_artifacts\work\jdk\jfr\jvm\TestDumpOnCrash\hs_err_pid23516.jfr
> #
> # An error report file with more information is saved as:
> # D:\utilities\jtreg\runtime_artifacts\work\jdk\jfr\jvm\TestDumpOnCrash\hs_err_pid23516.log
> #
> </pre>
> 
> To:
> <pre>
> # CreateCoredumpOnCrash turned off, no core file dumped
> #
> # JFR recording file will be written. Location: .\hs_err_pid23516.jfr <<--- not printing the full path as before
> #
> # An error report file with more information is saved as:
> # D:\utilities\jtreg\runtime_artifacts\work\jdk\jfr\jvm\TestDumpOnCrash\hs_err_pid23516.log
> #
> </pre>
> 
> 3. "Path to dump" description is vague. It is not clear that it must be directory, let alone that the directory must exist, and that you need write-access to it:
> 
> -XX:FlightRecorderOptions=maxchunksize=1m,repository=apa,dumppath=C:\temp\mydump.jfr ?
> 
> 4. Lack of troubleshooting information:
> 
> Any problematic path, for example specifying mounts that do not exist, wrong spelling, specifying a file path instead of a directory path, lack of security etc will get this:
> <pre>
> #
> # CreateCoredumpOnCrash turned off, no core file dumped
> #
> # The JFR repository may contain useful JFR files. Location: C:\Users\mgronlun\AppData\Local\Temp\2021_09_03_16_36_14_7524
> #
> # An error report file with more information is saved as:
> # D:\utilities\jtreg\runtime_artifacts\work\jdk\jfr\jvm\TestDumpOnCrash\hs_err_pid7524.log
> </pre>
> 
> Why was not my stated dumppath=<path> taken into account and used in creating an emergency dump file?
> 
> Thanks
> Markus

@mgronlun Thanks for your comment!

> 1. Modified default output in hs_err_pid.log file:

I fixed it.

> 1. Lack of troubleshooting information:

I checked dump path and write permission of it. However they might be changed when the dump happens, so I think it should be checked again in jfrEmergencyDump.cpp .
In hs_err log, it would fallback to current directory or temp directory if it cannot be written to specified path ( -XX:ErrorFile). Should emergency dump do so?

> 1. "Path to dump" description is vague. It is not clear that it must be directory, let alone that the directory must exist, and that you need write-access to it:

I guess you mentioned to the description in `JfrConfigureFlightRecorderDCmd::_dump_path`. As you know it is one-liner help message, so I think it is prefer to simple. In addition, I don't change semantics of `dumppath` (and don't change this description).

> 1. Mismatch in default behaviour for paths between option "repository" and "dumppath" for relative paths:

I cannot understand it well, but I guess you pointed "current directory" might be change since startup (start recording). But the same problem exists in current implementation because emergency dump path would be created by `os::get_current_directory()`, and it will be called when emergency dump happens.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5019