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

David Holmes david.holmes at oracle.com
Tue Dec 9 05:39:14 UTC 2014


Hi Thomas,

On 8/12/2014 8:27 PM, Thomas Stüfe wrote:
> Hi,
>
> I do not really like the handling of the leading pipe symbol:

To be fair to Yasumasa this aspect of the fix has been the same since 
Oct 15:

http://cr.openjdk.java.net/~ysuenaga/JDK-8059586/webrev.02/

and was not flagged.

> So, we read the core_pattern, and if the pipe symbol is detected, we
> write the core pattern minus the pipe symbol but plus a leading quote to
> the output; the leading quote then serves as a info to the layer above
> in os_posix.cpp to treat this case specially. This means the logic
> spills out of the platform dependend os_linux.cpp to shared code and
> this is also difficult to read.
>
> This comes from the fact that "get_core_path()" assumes the core file is
> written to the file system. I think it just does not fit anymore, better
> would be to replace it with something like
> "os::print_core_file_location(outputStream* os)", and the OS handles
> both core path retrieval and the printing. Because then the shared code
> does not need to know whether core file gets printed traditionally or
> piped to a executable or whatever.

This sounds like a refactoring that I suggested would be too disruptive.

http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-October/015547.html

http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-October/015557.html

http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-October/015573.html


David
-----

> Kind regards, Thomas
>
>
> On Mon, Dec 8, 2014 at 7:25 AM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
>     Hi Yasumasa,
>
>     I'm okay with these changes. Just a minor style nit (no need for
>     updated webrev) can you remove the blank lines in os_linux.cpp:
>
>     6011       }
>     6012
>     6013     }
>     6014
>     6015   }
>
>     6057       }
>     6058
>     6059     }
>     6060
>     6061   }
>
>     If anyone has any objections please raise them asap.
>
>     Thanks,
>     David
>
>
>     On 3/12/2014 12:30 AM, Yasumasa Suenaga wrote:
>
>         Hi David, Thomas,
>
>         I've uploaded new webrev:
>         http://cr.openjdk.java.net/~__ysuenaga/JDK-8059586/webrev.__04/
>         <http://cr.openjdk.java.net/~ysuenaga/JDK-8059586/webrev.04/>
>
>                      I want to rewrite a patch as below:
>
>                        - Use async signal safety functions.
>                          fopen -> open, fgets -> read, etc.
>
>
>             This is commendable if it is practical, but error reporting
>             already
>             does many, many things that are not async-signal safe, so
>             there is no
>             need to go to extreme measures here.
>
>
>         I've used async-signal safe functions as possible.
>
>
>                        - Use O_BUFLEN for buffer size.
>                          O_BUFLEN is defined to 2000 in ostream.hpp .
>                          This macro is used in various points.
>                 VMError::coredump_message is
>                          also defined with this value.
>
>
>                 I think PATH_MAX is fine. I think O_BUFLEN was
>                 originally used as a max.
>                 length of temporary buffers to assemble an output line.
>                 And then it
>                 spread a bit. But your intend is to hold a path and
>                 using PATH_MAX
>                 clearly documents this.
>
>
>         I've used PATH_MAX again.
>
>
>                 And, to really nitpick, right now you do not handle
>                 ERANGE with
>                 get_current_path() (if the provided buffer is too
>                 small), which is
>                 probably fine because it is improbable that a path is
>                 larger than
>                 PATH_MAX. But if you change the size of the buffer to
>                 something which
>                 may be smaller than PATH_MAX (O_BUFLEN),
>                 get_current_directory() may
>                 fail.
>
>
>         If get_current_path() call is failed in get_core_path(),
>         get_core_path()
>         returns immediately with 0.
>         Caller (check_or_create_dump()) handles this result as illegal
>         state.
>
>         get_current_path() calls getcwd() only and redirects result to
>         caller.
>         So result of this function is NULL, we can judge getcwd() was
>         finished with
>         error.
>         I think it is enough.
>
>
>                 I like your patch, I think it could be a nice time safer
>                 when
>                 core_pattern is something unusual. But I also see
>                 Staffans point of
>                 too-much-complexity. So I will keep out of this
>                 discussion until the
>                 real Reviewers decided what to do :)
>
>
>             I have a hard time evaluating the merits of the patch as I
>             don't work
>             in an environment where this extra info is needed. But I
>             take it on
>             good faith that it is useful for the context Yasumasa describes.
>
>
>         I want to suggest to Java user where coredump is.
>         Modern Linux distribution(s) contains ABRT.
>         OS can dump corefile automatically despite a lack of setting
>         coredump
>         resource by user.
>
>         I'm support engineer of Java. My customer says "coredump does
>         not found.",
>         but coredump is saved by ABRT.
>         Thus I want them to know "coredump is available" through stderr and
>         hs_err immediately.
>         I belive it is first step of troubleshoot.
>
>
>         Thanks,
>
>         Yasumasa
>
>
>         (2014/12/02 18:40), David Holmes wrote:
>
>             On 1/12/2014 10:57 PM, Thomas Stüfe wrote:
>
>                 Hi Yasumasa,
>
>                 On Mon, Dec 1, 2014 at 10:45 AM, Yasumasa Suenaga
>                 <yasuenag at gmail.com <mailto:yasuenag at gmail.com>
>                 <mailto:yasuenag at gmail.com <mailto:yasuenag at gmail.com>>>
>                 wrote:
>
>                      Hi Thomas, David,
>
>                      Sorry, I didn't think about async signal safety.
>
>                          That would work, VmError::report_and_die() is
>                 singlethreaded. At
>                          least the part which dumps out the core file name.
>
>
>                      I think that signal handler (in this case) may run
>                 concurrency with
>                      other thread.
>                      If another thread calls malloc(3) in JNI, C Heap
>                 corruption may
>                 occur.
>
>
>                 No, malloc(3) should be thread safe on our platforms.
>                 But this was not
>                 the point. If I understood David right, he suggested
>                 using a static
>                 buffer inside get_core_path() for assembling the core
>                 path, which would
>                 make get_core_path() thread-unsafe (multiple threads
>                 calling it would
>                 get garbled results). But as get_core_path() is only
>                 called from within
>                 VmError::report_and_die() and that section is only ever
>                 executed by one
>                 thread, Davids suggestion would still work.
>
>
>             Yes that is what I was suggesting.
>
>                      I want to rewrite a patch as below:
>
>                        - Use async signal safety functions.
>                          fopen -> open, fgets -> read, etc.
>
>
>             This is commendable if it is practical, but error reporting
>             already
>             does many, many things that are not async-signal safe, so
>             there is no
>             need to go to extreme measures here.
>
>                        - Use O_BUFLEN for buffer size.
>                          O_BUFLEN is defined to 2000 in ostream.hpp .
>                          This macro is used in various points.
>                 VMError::coredump_message is
>                          also defined with this value.
>
>
>                 I think PATH_MAX is fine. I think O_BUFLEN was
>                 originally used as a max.
>                 length of temporary buffers to assemble an output line.
>                 And then it
>                 spread a bit. But your intend is to hold a path and
>                 using PATH_MAX
>                 clearly documents this.
>                 And, to really nitpick, right now you do not handle
>                 ERANGE with
>                 get_current_path() (if the provided buffer is too
>                 small), which is
>                 probably fine because it is improbable that a path is
>                 larger than
>                 PATH_MAX. But if you change the size of the buffer to
>                 something which
>                 may be smaller than PATH_MAX (O_BUFLEN),
>                 get_current_directory() may
>                 fail.
>
>                 I like your patch, I think it could be a nice time safer
>                 when
>                 core_pattern is something unusual. But I also see
>                 Staffans point of
>                 too-much-complexity. So I will keep out of this
>                 discussion until the
>                 real Reviewers decided what to do :)
>
>
>             I have a hard time evaluating the merits of the patch as I
>             don't work
>             in an environment where this extra info is needed. But I
>             take it on
>             good faith that it is useful for the context Yasumasa describes.
>
>             David
>
>                 Kind Regards, Thomas
>
>


More information about the hotspot-runtime-dev mailing list