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

[Mikael Vidstedt]

New version:

http://cr.openjdk.java.net/~mikael/7129724/webrev.07/

Now checks return value from get_current_directory and get_core_path has 
been hoisted.

On 2012-06-27 14:50, David Holmes wrote:
> 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(...);"

snprintf (and therefore jio_snprintf) can return a value larger than 
bufferSize if the resulting string would have been longer than the 
supplied buffer. Or put differently, if the resulting string has been 
truncated jio_snprintf will still return the length of the full string.

Cheers,
Mikael

> 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
>>>>> ----
>>>>
>>>>