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

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