RFR (S): 7129724: Fix location of core file in dump message on macosx

[David Holmes]

On 28/06/2012 4:58 AM, Mikael Vidstedt wrote:
> You're right. I was set on maintaining the "correct" string, but
> relaxing that somewhat does make it easier.
>
> http://cr.openjdk.java.net/~mikael/7129724/webrev.02

Thanks Mikael!

One suggestion to address an existing latent bug in the code: check the 
return value from os::_get_current_directory is not NULL. In a generic 
sense we may have a path longer than the buffer being passed to 
getcwd(). I'd just add an assert for that.

Two possible small cleanups:

src/os/bsd/vm/os_bsd.cpp

How can n be greater than bufferSize? I would have thought you could 
just do "return jio_snprintf(...);"

src/os/posix/vm/os_posix.cpp

You could hoist out the get_core_path call to the start and so only 
write it once:

35 void os::check_or_create_dump(void* exceptionRecord, void* 
contextRecord, char* buffer, size_t bufferSize) {
37   struct rlimit rlim;
38   bool success;
36   int n = get_core_path(buffer, bufferSize);
39
40   if (getrlimit(RLIMIT_CORE, &rlim) != 0) {
42     jio_snprintf(buffer + n, bufferSize - n, "/core or core.%d (may 
not exist)", current_process_id());
...

Cheers,
David

>
> Thanks,
> Mikael
>
> On 6/26/2012 5:47 PM, David Holmes wrote:
>> Hi Mikael,
>>
>> On 27/06/2012 10:31 AM, Mikael Vidstedt wrote:
>>>
>>> David,
>>>
>>> Do you mean something like:
>>>
>>> http://cr.openjdk.java.net/~mikael/7129724/webrev.01
>>
>> Something like, but to simplify things I was thinking of just
>> returning the actual path component and have the check_or_create_dump
>> handle the "core or core.%d" part. No need to duplicate the pid
>> parsing logic then and the main part for linux/solaris is simply:
>>
>> int os::get_core_paths(char* buffer, size_t bufferSize) {
>> get_current_directory(buffer, bufferSize);
>> return strlen(buffer);
>> }
>>
>> (this is unchecked - I'm assuming things are properly nul-terminated
>> etc. Pity about the need for strlen, but then you could choose not to
>> return the length and instead do the strlen directly ie:
>>
>> ! os::get_core_paths(buffer, bufferSize);
>> n = strlen(buffer);
>> ! jio_snprintf(buffer + n, bufferSize - n, "...");
>>
>> either way works I guess)
>>
>> My suggestion does mean that the message is not as specific as it
>> might be if the name is always core.pid rather than core. But that
>> seems a very minor concern.
>>
>> BTW in current webrev this part needs fixing up:
>>
>> if (getrlimit(RLIMIT_CORE, &rlim) != 0) {
>> ! n = os::get_core_paths(buffer, bufferSize);
>> ! jio_snprintf(buffer + n, bufferSize - n, "%s/core or core.%d (may
>> not exist)");
>> success = true;
>>
>> You are no longer parsing anything for the %s and %d - I think you
>> really only want the "may not exist" part.
>>
>> Thanks,
>> David
>>
>>> Cheers,
>>> Mikael
>>>
>>> On 2012-06-25 23:31, David Holmes wrote:
>>>> Hi Mikael,
>>>>
>>>> On 26/06/2012 8:22 AM, Mikael Vidstedt wrote:
>>>>> If HotSpot crashes the dump message prints the default location of the
>>>>> generated core file. On other "posix" like OSes the path is normally
>>>>> <cwd>/core or core.<pid>, but on macosx the cores always end up in
>>>>> /cores/core.<pid>. This change aims to reflect that. Verified manually
>>>>> on Linux and macosx. Passes JPRT.
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~mikael/7129724/webrev.00
>>>>
>>>> I don't like this - sorry. The whole point of introducing
>>>> os_posix.cpp/hpp was to capture common "posix" functionality. The very
>>>> fact you need to add this change indicates this is not in fact "common
>>>> posix functionality". Seems to me that the default core location
>>>> should be a platform specific value:
>>>>
>>>> os::get_corefile_path(char* buf, int buflen)
>>>>
>>>> and then each os_*.cpp can define it as needed, with Solaris and Linux
>>>> simply delegating to the existing os::get_current_directory(buf, len).
>>>>
>>>> That aside, it really bugs me that BSD/OSX things are conditionalized
>>>> on __APPLE__ ! Should be BSD or OSX or our internally defined values
>>>> like TARGET_OS_FAMILY_xxx
>>>>
>>>> Cheers,
>>>> David
>>>> ----
>>>
>>>