RFR: JDK-8059586: hs_err report should treat redirected core pattern.
Thomas Stüfe
thomas.stuefe at gmail.com
Wed Nov 26 14:12:52 UTC 2014
Hi Yasumasa,
I am not a Reviewer. Barring the general decision of the real reviewers,
here are some thoughts:
os_linux.cpp
- jio_snprintf() returns -1 on truncation. n+=written may walk backwards. I
would probably check for (written >= 0) and also, at the start of the loop,
for (n < sizeof(core_path)).
- code is used in error reporting. I would be hesitant to create larger
buffers on the stack. malloc may be better.
- code does not detect truncation of core_path (unlikely but possible)
the rest is more matter of taste:
- I would prefer sizeof(core_path) over PATH_MAX at all places where you
refer to the size of the buffer. So you could make the buffer very small
and test e.g. how your code behaves with truncation.
- when reading /proc/sys/kernel/core_uses_pid, using fgetc instead of fgets
may be a tiny bit simpler.
Kind Regards, Thomas
On Wed, Nov 26, 2014 at 4:54 AM, Yasumasa Suenaga <yasuenag at gmail.com>
wrote:
> 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
> > >>>>>> >
> > >>>>>>
> >
> >
>
More information about the hotspot-runtime-dev
mailing list