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

Discussion:

Yasumasa Suenaga

2014-10-01 22:38:44 UTC

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

David Holmes

2014-10-07 06:42:56 UTC

Permalink

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

Post by Yasumasa Suenaga
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.
-------------
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

Yasumasa Suenaga

2014-10-07 10:48:53 UTC

Permalink

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.

Thanks,

Yasumasa

Post by David Holmes
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

"/usr/sbin/chroot /proc/%P/root /usr/libexec/abrt-hook-ccpp %s %c %p %u %g
%t e"

Post by Yasumasa Suenaga
-------------
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

David Holmes

2014-10-13 00:41:58 UTC

Permalink

Hi Yasumasa,

Post by Yasumasa Suenaga
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

Post by Yasumasa Suenaga
Thanks,
Yasumasa
2014/10/07 15:43 "David Holmes" <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

"/usr/sbin/chroot /proc/%P/root /usr/libexec/abrt-hook-ccpp %s %c %p
%u %g %t e"

Yasumasa Suenaga

2014-10-14 10:05:07 UTC

Permalink

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

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

I've fixed them.

Thanks,

Yasumasa

Hi Yasumasa,

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 ...
if(x){
should be
if (x) {
I also suggest saying "Core dumps may be processed with ..." rather than "treated".
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

"/usr/sbin/chroot /proc/%P/root /usr/libexec/abrt-hook-ccpp %s %c %p
%u %g %t e"

David Holmes

2014-10-15 00:41:35 UTC

Permalink

Post by Yasumasa Suenaga
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 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
-----

Post by Yasumasa Suenaga

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

I've fixed them.
Thanks,
Yasumasa

Hi Yasumasa,

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 ...
if(x){
should be
if (x) {
I also suggest saying "Core dumps may be processed with ..." rather than "treated".
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

Post by Yasumasa Suenaga
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)

Post by Yasumasa Suenaga
However, hs_err report cannot detect it.
-------------
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"

Yasumasa Suenaga

2014-10-15 14:13:12 UTC

Permalink

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

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

Post by Yasumasa Suenaga

Though I'm unclear whether it both invokes the program and creates a
core dump file; or just invokes the program?

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

I've fixed them.
Thanks,
Yasumasa

Hi Yasumasa,

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 ...
if(x){
should be
if (x) {
I also suggest saying "Core dumps may be processed with ..." rather than "treated".
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

Post by Yasumasa Suenaga
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)

Post by Yasumasa Suenaga
However, hs_err report cannot detect it.
-------------
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"