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-dev
mailing list