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

Staffan Larsen staffan.larsen at oracle.com
Tue Nov 25 09:15:34 UTC 2014 

src/os/bsd/vm/os_linux.cpp:
I’m inclined to think this is too complicated and hard to test and maintain (and I see no tests in the webrev). Could we not simplify this to print a helpful message instead? Something that prints the core_pattern and perhaps some of the values that could be used for substitution, but does not do the actual substitution? I think that would go a long way but be a lot more maintainable.

src/os/bsd/vm/os_bsd.cpp:
On OS X cores are by default written to /cores/core.<pid>. This is configureable with the kern.corefile sysctl variable, although it is rare to do so.

 /Staffan

> On 24 nov 2014, at 14:21, Yasumasa Suenaga <yasuenag at gmail.com> 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