RFR: JDK-8059586: hs_err report should treat redirected core pattern.
David Holmes
david.holmes at oracle.com
Wed Oct 15 00:41:35 UTC 2014
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