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

[Thomas Stüfe]

Hi,

I do not really like the handling of the leading pipe symbol:

So, we read the core_pattern, and if the pipe symbol is detected, we write
the core pattern minus the pipe symbol but plus a leading quote to the
output; the leading quote then serves as a info to the layer above in
os_posix.cpp to treat this case specially. This means the logic spills out
of the platform dependend os_linux.cpp to shared code and this is also
difficult to read.

This comes from the fact that "get_core_path()" assumes the core file is
written to the file system. I think it just does not fit anymore, better
would be to replace it with something like
"os::print_core_file_location(outputStream* os)", and the OS handles both
core path retrieval and the printing. Because then the shared code does not
need to know whether core file gets printed traditionally or piped to a
executable or whatever.

Kind regards, Thomas


On Mon, Dec 8, 2014 at 7:25 AM, David Holmes <david.holmes at oracle.com>
wrote:

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