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

David Holmes david.holmes at oracle.com
Tue Nov 25 08:38:58 UTC 2014 

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