On Wed, 9 Apr 2025 06:43:20 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> Kevin Walls has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - length checking update
>> - Chris feedback
>
> src/hotspot/share/services/heapDumper.cpp line 2760:
>
>> 2758: if (dump_file_seq == 0) { // first time in, we initialize base_path
>> 2759: // Set base path (name or directory, default or custom, without
>> seq no), doing %p substitution.
>> 2760: const char *path_src = (HeapDumpPath != nullptr && HeapDumpPath[0]
>> != '\0') ? HeapDumpPath : dump_file_name;
>
> Why do you expand the dump file name here?
>
> If you want to minimize the expand calls, you could:
>
> - append the unexpanded dump_file_name to the unexpanded HeapDumpPath
> - expand
> - create dir (extract the directory name by temporarily setting the the last
> '/' to '\0; create dir; restore '/')
> now you are done.
It concerned me that we presume there is no %p in the directory/path. It might
be tricky (get the pid, create a directory, before the first heap dump), but
it's possible.
Trying to use an existing directory with a literal %p in it will fail, but we
can't have this both ways, and should either honour the %p substitution fully,
or not.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24482#discussion_r2034820138
