RFR: JDK-8059586: hs_err report should treat redirected core pattern.

[Yasumasa Suenaga]

Hi David, Thomas,

I've uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8059586/webrev.04/

>>     I want to rewrite a patch as below:
>>
>>       - Use async signal safety functions.
>>         fopen -> open, fgets -> read, etc.
>
> This is commendable if it is practical, but error reporting already does many, many things that are not async-signal safe, so there is no need to go to extreme measures here.

I've used async-signal safe functions as possible.


>>       - Use O_BUFLEN for buffer size.
>>         O_BUFLEN is defined to 2000 in ostream.hpp .
>>         This macro is used in various points. VMError::coredump_message is
>>         also defined with this value.
>>
>>
>> I think PATH_MAX is fine. I think O_BUFLEN was originally used as a max.
>> length of temporary buffers to assemble an output line. And then it
>> spread a bit. But your intend is to hold a path and using PATH_MAX
>> clearly documents this.

I've used PATH_MAX again.


>> And, to really nitpick, right now you do not handle ERANGE with
>> get_current_path() (if the provided buffer is too small), which is
>> probably fine because it is improbable that a path is larger than
>> PATH_MAX. But if you change the size of the buffer to something which
>> may be smaller than PATH_MAX (O_BUFLEN), get_current_directory() may fail.

If get_current_path() call is failed in get_core_path(), get_core_path()
returns immediately with 0.
Caller (check_or_create_dump()) handles this result as illegal state.

get_current_path() calls getcwd() only and redirects result to caller.
So result of this function is NULL, we can judge getcwd() was finished with
error.
I think it is enough.


>> I like your patch, I think it could be a nice time safer when
>> core_pattern is something unusual. But I also see Staffans point of
>> too-much-complexity. So I will keep out of this discussion until the
>> real Reviewers decided what to do :)
>
> I have a hard time evaluating the merits of the patch as I don't work in an environment where this extra info is needed. But I take it on good faith that it is useful for the context Yasumasa describes.

I want to suggest to Java user where coredump is.
Modern Linux distribution(s) contains ABRT.
OS can dump corefile automatically despite a lack of setting coredump
resource by user.

I'm support engineer of Java. My customer says "coredump does not found.",
but coredump is saved by ABRT.
Thus I want them to know "coredump is available" through stderr and hs_err immediately.
I belive it is first step of troubleshoot.


Thanks,

Yasumasa


(2014/12/02 18:40), David Holmes wrote:
> On 1/12/2014 10:57 PM, Thomas Stüfe wrote:
>> Hi Yasumasa,
>>
>> On Mon, Dec 1, 2014 at 10:45 AM, Yasumasa Suenaga <yasuenag at gmail.com
>> <mailto:yasuenag at gmail.com>> wrote:
>>
>>     Hi Thomas, David,
>>
>>     Sorry, I didn't think about async signal safety.
>>
>>         That would work, VmError::report_and_die() is singlethreaded. At
>>         least the part which dumps out the core file name.
>>
>>
>>     I think that signal handler (in this case) may run concurrency with
>>     other thread.
>>     If another thread calls malloc(3) in JNI, C Heap corruption may occur.
>>
>>
>> No, malloc(3) should be thread safe on our platforms. But this was not
>> the point. If I understood David right, he suggested using a static
>> buffer inside get_core_path() for assembling the core path, which would
>> make get_core_path() thread-unsafe (multiple threads calling it would
>> get garbled results). But as get_core_path() is only called from within
>> VmError::report_and_die() and that section is only ever executed by one
>> thread, Davids suggestion would still work.
>
> Yes that is what I was suggesting.
>
>>     I want to rewrite a patch as below:
>>
>>       - Use async signal safety functions.
>>         fopen -> open, fgets -> read, etc.
>
> This is commendable if it is practical, but error reporting already does many, many things that are not async-signal safe, so there is no need to go to extreme measures here.
>
>>       - Use O_BUFLEN for buffer size.
>>         O_BUFLEN is defined to 2000 in ostream.hpp .
>>         This macro is used in various points. VMError::coredump_message is
>>         also defined with this value.
>>
>>
>> I think PATH_MAX is fine. I think O_BUFLEN was originally used as a max.
>> length of temporary buffers to assemble an output line. And then it
>> spread a bit. But your intend is to hold a path and using PATH_MAX
>> clearly documents this.
>> And, to really nitpick, right now you do not handle ERANGE with
>> get_current_path() (if the provided buffer is too small), which is
>> probably fine because it is improbable that a path is larger than
>> PATH_MAX. But if you change the size of the buffer to something which
>> may be smaller than PATH_MAX (O_BUFLEN), get_current_directory() may fail.
>>
>> I like your patch, I think it could be a nice time safer when
>> core_pattern is something unusual. But I also see Staffans point of
>> too-much-complexity. So I will keep out of this discussion until the
>> real Reviewers decided what to do :)
>
> I have a hard time evaluating the merits of the patch as I don't work in an environment where this extra info is needed. But I take it on good faith that it is useful for the context Yasumasa describes.
>
> David
>
>> Kind Regards, Thomas
>>