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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: JDK-8059586: hs_err report should treat redirected core pattern.
From:       Yasumasa Suenaga <yasuenag () gmail ! com>
Date:       2014-11-29 15:44:30
Message-ID: 5479E9DE.7070703 () gmail ! com
[Download RAW message or body]

Hi all,


Thank you for checking my patch!
I've uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8059586/webrev.03/hotspot.patch

David:
> 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.

I've fixed.


Staffan:
> src/os/bsd/vm/os_linux.cpp:
> Could we not simplify this to print a helpful message instead?

Most of case in Linux, I think that core image name is "core.<pid>" .
In other case which except pipe redirection, I guess that user defines it.
Thus I print string in kernel.core_pattern directly.

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

Thank you!
I changed path to "/cores/core.<pid>" .


Thomas:
> - 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.

I've fixed them.

> - code does not detect truncation of core_path (unlikely but possible)

Do you mean variable name?
"core_path" in my patch stores /proc/sys/kernel/core_pattern .
Length of kernel.core_pattern is defined 128 chars in Linux Kernel Documentation.
https://www.kernel.org/doc/Documentation/sysctl/kernel.txt

Thus length of core_path (129 chars) is enough.

> - when reading /proc/sys/kernel/core_uses_pid, using fgetc instead of fgets may be \
> a tiny bit simpler.

I changed to use fgetc() .


Thanks,

Yasumasa


(2014/11/26 23:12), Thomas Stüfe wrote:
> 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@gmail.com \
> <mailto:yasuenag@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@oracle.com \
> <mailto:staffan.larsen@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@gmail.com \
> > > <mailto:yasuenag@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@oracle.com \
> > > > > > > > <mailto:david.holmes@oracle.com> <mailto:david.holmes@oracle.com \
> > > > > > > > <mailto:david.holmes@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
> > > > > > > > > 
> > > > > > > > 
> > 
> > 
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic