RFR: JDK-8059586: hs_err report should treat redirected core pattern.
Yasumasa Suenaga
yasuenag at gmail.com
Mon Nov 24 13:21:41 UTC 2014
Hi all,
I've uploaded webrev for this issue about a month ago.
Could you review it and sponsor it?
Thanks,
Yasumasa
On 10/15/2014 11:13 PM, Yasumasa Suenaga wrote:
> 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-dev
mailing list