RFR: JDK-8059586: hs_err report should treat redirected core pattern.
Yasumasa Suenaga
yasuenag at gmail.com
Wed Nov 26 03:39:33 UTC 2014
Hi David,
Thank you for reviewing!
I will fix it after discussion with Staffan.
Thanks
Yasumasa
2014/11/25 17:39 "David Holmes" <david.holmes at oracle.com>:
> Sorry Yasumasa, this fell off my radar and I was hoping for others to
> comment. We still need a second reviewer.
>
> The change in:
> src/os/aix/vm/os_aix.cpp
> src/os/solaris/vm/os_solaris.cpp
>
> jio_snprintf(buffer, bufferSize, "%s/core or core.%d",
> current_process_id());
>
> has no argument for the %s - presumably p was intended.
>
> Thanks,
> David
>
> On 24/11/2014 11:21 PM, Yasumasa Suenaga wrote:
>
>> 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