RFR: 8234741: enhance os::get_core_path on macOS

[gerard ziemski]

Looks good. Thank you!


On 11/28/19 5:09 AM, Baesken, Matthias wrote:
> Thanks for the review.
>
>> Please also add os:: to the other call to current_process_id() in line 3787
> Okay.
>
> Gerard, may I add you as reviewer ?
>
>
> Best regards, Matthias
>
>
>
>> -----Original Message-----
>> From: Langer, Christoph <christoph.langer at sap.com>
>> Sent: Donnerstag, 28. November 2019 12:03
>> To: Baesken, Matthias <matthias.baesken at sap.com>; gerard ziemski
>> <gerard.ziemski at oracle.com>
>> Cc: hotspot-dev developers <hotspot-dev at openjdk.java.net>
>> Subject: RE: RFR: 8234741: enhance os::get_core_path on macOS
>>
>> Hi Matthias,
>>
>> this looks good to me.
>>
>> Please also add os:: to the other call to current_process_id() in line 3787 (the
>> else branch). No need for another webrev, though.
>>
>> /Christoph
>>
>>> -----Original Message-----
>>> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf
>> Of
>>> Baesken, Matthias
>>> Sent: Mittwoch, 27. November 2019 11:09
>>> To: gerard ziemski <gerard.ziemski at oracle.com>
>>> Cc: hotspot-dev developers <hotspot-dev at openjdk.java.net>
>>> Subject: RE: RFR: 8234741: enhance os::get_core_path on macOS
>>>
>>> Hi Gerard, thanks for your input .
>>>
>>> New webrev :
>>>
>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8234741.1/
>>>
>>>
>>> Best regards, Matthias
>>>
>>>
>>>> hi Matthias,
>>>>
>>>> I had to look up "kern.corefile" option, of which I was not previously
>>>> aware - it looks like a nice enhancement.
>>>>
>>>> I'd like to suggest a few small cleanups though:
>>>>
>>>> #1 add "os::" prefix to "current_process_id()" call
>>>> #2 restrict the scope of "os::current_process_id()" to only the branch
>>>> of "if" that needs it
>>>> #3 expand on the "tail" comment a bit to explain why it might be needed
>>>>
>>>> Perhaps something like this:
>>>>
>>>>     char coreinfo[MAX_PATH];
>>>>      size_t sz = sizeof(coreinfo);
>>>>      int ret = sysctlbyname("kern.corefile", coreinfo, &sz, NULL, 0);
>>>>      if (ret == 0) {
>>>>        char *pid_pos = strstr(coreinfo, "%P");
>>>>        const char* tail = (pid_pos != NULL) ? (pid_pos + 2) : ""; // skip
>>>> over the "%P" to preserve any optional custom user pattern (i.e. %N,
>> %U)
>>>>        if (pid_pos != NULL) {
>>>>          *pid_pos = '\0';
>>>>          n = jio_snprintf(buffer, bufferSize, "%s%d%s", coreinfo,
>>>> os::current_process_id(), tail);
>>>>        } else {
>>>>          n = jio_snprintf(buffer, bufferSize, "%s", coreinfo);
>>>>        }
>>>>      }
>>>>
>>>> BTW. I'm glad you agree to remove the unrelated AWT change from this
>> fix
>>>> and let the client team handle it.
>>>>
>>>>