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

[Thomas Stuefe]

On Sat, 20 Jul 2024 07:38:25 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> 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.
>
> Yes. In this file, other commands use `fatal` only where reading the hard-coded default values - in the various `init_...` functions. Hard-coded values should be valid, obviously, otherwise the JVM developer messed up. Other values are passed in by the end user via jcmd and should not crash the JVM.

I see the prevalent way to deal with runtime parse errors is to throw a java exception. That exception later is caught in the command processing loop at the entrance of the attach listener thread.

So, @SoniaZaldana, I would do this here too - when in Rome...

But is this not unnecessarily complex? It requires the AttachListener to be a java thread when in fact it does need no java - we just misuse java exception handling as a way to pass error information up the stack, with the simple ultimate goal of writing error information into the outputStream to be sent to the caller. We might just as well pass the outputStream* to the parse_xxx functions as third argument, and write directly and return some error code. This would make the attach listener thread a lot less dependent on Java and more robust - at least for jcmds that don't need Java (which jcmds need java?). 

After all, the attach listener is supposed to be super robust and always work even if the JVM misbehaves. @dholmes-ora @lmesnik what do you guys think, should we change that? (obviously in a different RFE)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685304522