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

[Thomas Stuefe]

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

>> Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Missing copyright header update
>
> src/hotspot/share/services/diagnosticArgument.hpp line 66:
> 
>> 64:   public:
>> 65:     char *_name;
>> 66: };
> 
> Something is off about this. What is the lifetime of this object?
> 
> You don't free it. Running a command in a loop will consume C-heap (you can check this with NMT: `jcmd VM.native_memory baseline`, then run a command 100 times, then `jcmd VM.native_memory summary.diff` will show you the leak in mtInternal.
> 
> I would probably just inline the string. E.g.
> 
> 
> struct FileArgument {
>   char name[max name len]
> };
> 
> 
> FileArguments sits as member inside DCmdArgument. DCmdArgument or DCmdArgumentWithParser sits as member in the various XXXDCmd classes. 
> 
> Those are created in DCmdFactory::create_local_DCmd(). Which is what, a static global list? So we only have one global XXXDCmd object instance per command, but for each command invocation re-parse the argument values? What a weird concept.
> 
> Man, this coding is way too convoluted for a little parser engine :( 
> 
> But anyway, inlining the filename array into FileArgument should be probably fine from a size standpoint. I would, however, not use JVM_MAXPATHLEN or anything that depends ultimately on PATH_MAX from system headers. We don't want the object to consume e.g. an MB if some crazy platform defines PATH_MAX as 1MB. Therefore I would use e.g. 1024 as limit for the path name.
> 
> (Note that PATH_MAX is an illusion anyway, there is never a guarantee that a path is smaller than that limit... See this good article: https://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html)

Note that the reason for the leak is probably the fact that you don't clear old values on parse_value. See e.g. how char* does it. However, since you allocate with a constant size anyway, the buffer size never changes, you could just as well either follow my advice above (inlining), or just re-use the existing pointer.

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

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