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

[David Holmes]

Hi Yasumasa,

I'm okay with these changes. Just a minor style nit (no need for updated 
webrev) can you remove the blank lines in os_linux.cpp:

6011       }
6012
6013     }
6014
6015   }

6057       }
6058
6059     }
6060
6061   }

If anyone has any objections please raise them asap.

Thanks,
David

On 3/12/2014 12:30 AM, Yasumasa Suenaga wrote:
> 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
>>>