On Fri, 19 Jul 2024 14:07:12 GMT, Sonia Zaldana Calles <szald...@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
