RFR: 8275375: [REDO] JDK-8271949 dumppath in -XX:FlightRecorderOptions does not affect

[Yasumasa Suenaga]

On Tue, 19 Oct 2021 03:43:01 GMT, Erik Gahlin <egahlin at openjdk.org> wrote:

>>> There are three ways setDumpPath is used:
>>> 
>>> 1. When JFR is initialized
>>> 2. jcmd JFR.configure dumppath=
>>> 3. -XX:FllightRecorderOptions:dumppath=
>>> 
>>> If something goes wrong (IOException/InvalidPathException) when doing 2 or 3, users should ideally be informed, so they can correct it, at command-line or jcmd. Perhaps throw DCmdException?
>> 
>> Agree, we can use DCmdException if IOE happens. However we need to concider 1 because some processes might be run on "/" with unprivileged user.
>> 
>>> Do we really need to resolve for 1? Can we handle it in native, similar to what we already do with hs_err_pid (which is not resolved in Java). For example, null could be used to indicate that the working directory should be used.
>> 
>> In hs_err log, the path would not resolved, it uses `os::get_current_directory()` at `VMError::prepare_log_file()` when -XX:ErrorFile is not set. For example, we can see following message on the console when we set -XX:ErrorFile=error.log, and error.log is located at current directory.
>> 
>> 
>> # An error report file with more information is saved as:
>> # error.log
>> 
>> 
>> I'm not particular path resolution if we can ignore original behavior (resolved dump path will also be shown at UL). We will not have to think about SecurityException if we don't need to resolve dump path at `setDumpPath()`. In addition we can implement it more simple. So I agree with you to be honest that to pass `null`.
>
>> > There are three ways setDumpPath is used:
>> > 
>> > 1. When JFR is initialized
>> > 2. jcmd JFR.configure dumppath=
>> > 3. -XX:FllightRecorderOptions:dumppath=
>> > 
>> > If something goes wrong (IOException/InvalidPathException) when doing 2 or 3, users should ideally be informed, so they can correct it, at command-line or jcmd. Perhaps throw DCmdException?
>> 
>> Agree, we can use DCmdException if IOE happens. However we need to concider 1 because some processes might be run on "/" with unprivileged user.
>> 
>> > Do we really need to resolve for 1? Can we handle it in native, similar to what we already do with hs_err_pid (which is not resolved in Java). For example, null could be used to indicate that the working directory should be used.
>> 
>> In hs_err log, the path would not resolved, it uses `os::get_current_directory()` at `VMError::prepare_log_file()` when -XX:ErrorFile is not set. For example, we can see following message on the console when we set -XX:ErrorFile=error.log, and error.log is located at current directory.
>> 
>> ```
>> # An error report file with more information is saved as:
>> # error.log
>> ```
>> 
>> I'm not particular path resolution if we can ignore original behavior (resolved dump path will also be shown at UL). We will not have to think about SecurityException if we don't need to resolve dump path at `setDumpPath()`. In addition we can implement it more simple. So I agree with you to be honest that to pass `null`.
> 
> I'm not sure I follow completely, but how about this:
> 
> 1) Remove setDumpPath(DEFAULT_DUMP_PATH) in Options::reset(). If no path has been set, os::get_current_directory() will be used when doing an emergency dump.
> 2) Use the SecuritySupport::canWrite check, but instead of logging, let setDumpPath throw IOException. 
> 3) Catch IOException in DCmdConfigure and throw DCmdExceotion, similar to what is done with repository path
> 
> Would that work?

@egahlin Thanks for your comment!

> 1. Remove setDumpPath(DEFAULT_DUMP_PATH) in Options::reset(). If no path has been set, os::get_current_directory() will be used when doing an emergency dump.

`Options::getDumpPath()` would be called at `FlightRecorder::getFlightRecorder` to log the configuration. So we need to replace `null` to current working directory. My new change will do so.

> 2. Use the SecuritySupport::canWrite check, but instead of logging, let setDumpPath throw IOException.

I use `SecuritySupport::isWritable` because `SecuritySupport` does not have `canWrite` and we can use `isWritable` for this purpose.
We will throw IllegalArgumentException when IOException happens because IOE is check exception. 

> 3. Catch IOException in DCmdConfigure and throw DCmdExceotion, similar to what is done with repository path

DCmdConfigure would throw DCmdException when it catches IAE because I don't want to change method declaration (`setDumpPath()`).


New commit works expectedly, and it passed all jdk/jfr tests on Linux x64.

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

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