RFR: JDK-8059586: hs_err report should treat redirected core pattern.
Yasumasa Suenaga
yasuenag at gmail.com
Wed Oct 15 14:13:12 UTC 2014
Hi David,
I've uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8059586/webrev.02/
> I wasn't suggesting that you make such a change though because it is large and disruptive.
> Unfactoring check_or_create_dump is a step backwards in terms of code sharing.
I restored check_or_create_dump() to os_posix.cpp .
And I changed get_core_path() to create message which represents core dump path
(including filename) in each OS.
> Expanding the get_core_path in os_linux.cpp to handle the core_pattern may be okay (but I don't know enough about it to validate everything).
I implemented all parameters in Linux kernel documentation:
https://www.kernel.org/doc/Documentation/sysctl/kernel.txt
So I think that parameters which are processed are enough.
Thanks,
Yasumasa
(2014/10/15 9:41), David Holmes wrote:
> On 14/10/2014 8:05 PM, Yasumasa Suenaga wrote:
>> Hi David,
>>
>> Thank you for comments!
>> I've uploaded new webrev. Could you review it again?
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8059586/webrev.01/
>>
>> I am an author of jdk9. So I cannot commit it.
>> Could you be a sponsor for this enhancement?
>>
>>
>>> In which case that should be handled by the linux specific
>>> get_core_path() function.
>>
>> Agree.
>> So I implemented it in os_linux.cpp .
>> But part of format characters (%P: global pid, %s: signal, %t dump time)
>> are not processed
>> in this function because I think these parameters are difficult to
>> handle in it.
>>
>> %P: I could not find API for this.
>> %s: We have to change arguments of get_core_path() .
>> %t: This parameter means timestamp of coredump. It is decided in Kernel.
>>
>>
>>> Fixing this means changing all the os_posix using platforms. But your
>>> patch is not about this part. :)
>>
>> I moved os::check_or_create_dump() to each OS implementations (AIX, BSD,
>> Solaris, Linux) .
>> So I can write Linux specific code to check_or_create_dump() .
>> As a result, I could remove "#ifdef LINUX" from os_posix.cpp :-)
>
> I wasn't suggesting that you make such a change though because it is large and disruptive. The simple handling of the | part of core_pattern was basically ok. Expanding the get_core_path in os_linux.cpp to handle the core_pattern may be okay (but I don't know enough about it to validate everything). Unfactoring check_or_create_dump is a step backwards in terms of code sharing.
>
> Sorry this has grown too large for me to deal with right now.
>
> David
> -----
>
>>
>>> Though I'm unclear whether it both invokes the program and creates a
>>> core dump file; or just invokes the program?
>>
>> If '|' is set, Linux kernel will just redirect core image to user process.
>> Kernel documentation says as below:
>> ------------
>> . If the first character of the pattern is a '|', the kernel will treat
>> the rest of the pattern as a command to run. The core dump will be
>> written to the standard input of that program instead of to a file.
>> ------------
>>
>> And implementation of coredump (do_coredump()) follows to it.
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/coredump.c
>>
>>
>> In case of ABRT, ABRT dumps core image to default location
>> (<CWD>/core.<PID>)
>> if user set unlimited to resource limit of core (ulimit -c) .
>> https://github.com/abrt/abrt/blob/master/src/hooks/abrt-hook-ccpp.c
>>
>>
>>> A few style nits - you need spaces around keywords and before braces
>>> I also suggest saying "Core dumps may be processed with ..." rather
>>> than "treated".
>>> And as you don't do anything in the non-redirect case I suggest
>>> collapsing this:
>>
>> I've fixed them.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> (2014/10/13 9:41), David Holmes wrote:
>>> Hi Yasumasa,
>>>
>>> On 7/10/2014 8:48 PM, Yasumasa Suenaga wrote:
>>>> Hi David,
>>>>
>>>> Sorry for my English.
>>>>
>>>> I want to propose that JVM should create message according to core
>>>> pattern (/proc/sys/kernel/core_pattern) .
>>>> So I filed it to JBS and created a patch.
>>>
>>> So I've had a quick look at this core_pattern business and it seems to
>>> me that there are two aspects to this.
>>>
>>> First, without the leading |, the entry in the core_pattern file is a
>>> naming pattern for the core file. In which case that should be handled
>>> by the linux specific get_core_path() function. Though that in itself
>>> can't fully report the expected name, as part of it is provided in the
>>> shared code in os::check_or_create_dump. Fixing this means changing
>>> all the os_posix using platforms. But your patch is not about this
>>> part. :)
>>>
>>> Second, with a leading | the core_pattern is actually the name of a
>>> program to execute when the program is about to core dump, and that is
>>> what you report with your patch. Though I'm unclear whether it both
>>> invokes the program and creates a core dump file; or just invokes the
>>> program?
>>>
>>> So with regards to this second part your patch seems functionally ok.
>>> I do dislike having a big chunk of linux specific code in this "posix"
>>> support file but ...
>>>
>>> A few style nits - you need spaces around keywords and before braces eg:
>>>
>>> if(x){
>>>
>>> should be
>>>
>>> if (x) {
>>>
>>> I also suggest saying "Core dumps may be processed with ..." rather
>>> than "treated".
>>>
>>> And as you don't do anything in the non-redirect case I suggest
>>> collapsing this:
>>>
>>> 83 is_redirect = core_pattern[0] == '|';
>>> 84 }
>>> 85
>>> 86 if(is_redirect){
>>> 87 jio_snprintf(buffer, bufferSize,
>>> 88 "Core dumps may be treated with \"%s\"",
>>> &core_pattern[1]);
>>> 89 }
>>>
>>> to just
>>>
>>> 83 if (core_pattern[0] == '|') { // redirect
>>> 84 jio_snprintf(buffer, bufferSize, "Core dumps may be
>>> processed with \"%s\"", &core_pattern[1]);
>>> 85 }
>>> 86 }
>>>
>>> Comments from other runtime folk appreciated.
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>> 2014/10/07 15:43 "David Holmes" <david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>>:
>>>>
>>>> Hi Yasumasa,
>>>>
>>>> I'm sorry but I don't understand what you are proposing. When you
>>>> say
>>>> "treat" do you mean "create"? Otherwise what do you mean by
>>>> "treated"?
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 2/10/2014 8:38 AM, Yasumasa Suenaga wrote:
>>>> > I'm in Hackergarten @ JavaOne :-)
>>>> >
>>>> >
>>>> > Hi all,
>>>> >
>>>> > I would like to enhance the messages in hs_err report.
>>>> > Modern Linux kernel can treat core dump with user process
>>>> (e.g. ABRT)
>>>> > However, hs_err report cannot detect it.
>>>> >
>>>> > I think that hs_err report should output messages as below:
>>>> > -------------
>>>> > Failed to write core dump. Core dumps may be treated with
>>>> "/usr/sbin/chroot /proc/%P/root /usr/libexec/abrt-hook-ccpp %s %c %p
>>>> %u %g %t e"
>>>> > -------------
>>>> >
>>>> > I've uploaded webrev of this enhancement.
>>>> > Could you review it?
>>>> >
>>>> > http://cr.openjdk.java.net/~ysuenaga/JDK-8059586/webrev.00/
>>>> >
>>>> > This patch works fine on Fedora20 x86_64.
>>>> >
>>>> >
>>>> >
>>>> > Thanks,
>>>> >
>>>> > Yasumasa
>>>> >
>>>>
More information about the hotspot-runtime-dev
mailing list