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

[Leonid Mesnik]

On Fri, 19 Jul 2024 14:07:12 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 incrementally with one additional commit since the last revision:
> 
>   Missing copyright header update

Thanks for updating the fix. The new  version looks moistly good. I added a few small comments.

src/hotspot/share/prims/wbtestmethods/parserTests.cpp line 132:

> 130:    } else if (strcmp(type, "FILE") == 0) {
> 131:      DCmdArgument<FileArgument> *argument =
> 132:       new DCmdArgument<FileArgument>(name, desc, "FILE", mandatory);

Please check indentation.

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

> 356: template <> void DCmdArgument<MemorySizeArgument>::destroy_value() { }
> 357: 
> 358: template <>

The common style here is to place in the single line 'template<> and other part of declaration.

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

> 364:     _value._name = NEW_C_HEAP_ARRAY(char, JVM_MAXPATHLEN, mtInternal);
> 365:     if (!Arguments::copy_expand_pid(str, len, _value._name, JVM_MAXPATHLEN)) {
> 366:       fatal("Invalid file path: %s", str);

As I understand the 'copy_expand_pid' might fail if very long line is used. This cause jvm crash.,
So there is possibility that user might crash jvm accidentally invoking jcmd command. 
It doesn't look safe, I believe it would be better to throw Exception like for any other invalid command, see
" THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),"

The 'fatal" owuld make sense only if failing of 'copy_expand_pid' means some unrecoverable jvm bug.

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

Changes requested by lmesnik (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20198#pullrequestreview-2189044201
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1684887604
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1684892964
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1684923626