RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v9]

[Thomas Stuefe]

On Tue, 23 Jul 2024 17:57:04 GMT, Sonia Zaldana Calles <szaldana at openjdk.org> wrote:

>> Hi all, 
>> 
>> This PR addresses [8334492](https://bugs.openjdk.org/browse/JDK-8334492) enabling jcmd diagnostic commands that issue an output file to accept the `%p` pattern in the file name and substitute it for the PID. 
>> 
>> This PR addresses the following diagnostic commands: 
>> - [x] Compiler.perfmap 
>> - [x] GC.heap_dump
>> - [x] System.dump_map
>> - [x] Thread.dump_to_file
>> - [x] VM.cds
>> 
>> Note that some jcmd diagnostic commands already enable this functionality (`JFR.configure, JFR.dump, JFR.start and JFR.stop`). 
>> 
>> I propose opening a separate issue to track updating the man page similarly to how it’s done for the JFR diagnostic commands. For example, 
>> 
>> 
>> filename         (Optional) Name of the file to which the flight recording data is
>>                    written when the recording is stopped. If no filename is given, a
>>                    filename is generated from the PID and the current date and is
>>                    placed in the directory where the process was started. The
>>                    filename may also be a directory in which case, the filename is
>>                    generated from the PID and the current date in the specified
>>                    directory. (STRING, no default value)
>> 
>>                    Note: If a filename is given, '%p' in the filename will be
>>                    replaced by the PID, and '%t' will be replaced by the time in
>>                    'yyyy_MM_dd_HH_mm_ss' format.
>> 
>> 
>> Unfortunately, per [8276265](https://bugs.openjdk.org/browse/JDK-8276265), sources for the jcmd manpage remain in Oracle internal repos so this PR can’t address that. 
>> 
>> Testing: 
>> 
>> - [x] Added test case passes. 
>> - [x] Modified existing VM.cds tests to also check for `%p` filenames. 
>> 
>> Looking forward to your comments and addressing any diagnostic commands I might have missed (if any). 
>> 
>> Cheers, 
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits:
> 
>  - Merge master
>  - Fixing formatting
>  - Inlining buffer and making field private
>  - Reverting to functional changes in parserTests.cpp
>  - Error messaging format
>  - Fixing memory leak
>  - Fixing pointer style, s/NULL/nullptr, and exception
>  - Cleaning up parserTests.cpp
>  - Missing copyright header update
>  - Adding tests for file dcmd argument
>  - ... and 5 more: https://git.openjdk.org/jdk/compare/2f2223d7...52ca557d

src/hotspot/share/services/diagnosticArgument.cpp line 365:

> 363:     if (!_value.parse_value(str, len)) {
> 364:       stringStream error_msg;
> 365:       error_msg.print("Invalid file path: %s", str);

In all likelyhood the only reason Argument::copy_expand... is ever going to fail would be if the expanded string would not fit the buffer in FileArgument. I'd consider a clearer warning here, therefore ("File path invalid or too long: ")

src/hotspot/share/services/diagnosticCommand.cpp line 1018:

> 1016:   // of the default, not the actual default.
> 1017:   FileArgument file_arg = _filename.value();
> 1018:   const char *file = _filename.is_set() ? file_arg.get() : nullptr;

Style nit: const char*, not const char *

src/hotspot/share/services/diagnosticCommand.cpp line 1197:

> 1195: void SystemDumpMapDCmd::execute(DCmdSource source, TRAPS) {
> 1196:   FileArgument file_arg = _filename.value();
> 1197:   const char *name = file_arg.get();

pointer style

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1689204690
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1689206254
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1689208226