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

Wed Nov 26 03:54:48 UTC 2014

Hi Staffan,

Thank you for reviewing!

os_linux.cpp:
I want to print coredump location correctly to hs_err. So I want to output
whether coredump is processed in other process or is written to file.
If os::get_core_path() should be more simply, I will print raw string in
core_pattern.

os_bsd.cpp:
I don't have OS X. So I cannot check it.
I am focusing Linux in this enhancement. Could you file it as another
enhancement if it need?

Thanks,

Yasumasa

 2014/11/25 18:15 "Staffan Larsen" <staffan.larsen at oracle.com>:

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