RFR [XS] 8210964: add more ld preloading info to hs_error file on Linux

[Thomas Stüfe]

On Fri, Sep 21, 2018 at 6:47 PM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> Hi Matthias,
>
> in the posted patch the new function is never called, or am I mistaken?
>

Scratch that, I was being blind.

What I do not like is the very generic name of the function. It does
one very specific thing, so could we name it accordingly, e.g.
"os::Linux::print_system_config_info" ->
"os::Linux::print_ld_preload_file"?

Also, I personally would not expose it via os_linux.hpp. I would just
put it as a file scope static function into os_linux.cpp. I see you
follow the example of others here (os::Linux::print_proc_sys_info etc)
but there too I feel this is a bad pattern and those functions do not
belong into an external header.

2140 st->print_cr("/etc/ld.so.preload (system-wide LD_PRELOAD config file):");

I would shorten that to just "ld.so.preload:" or "/etc/ld.so.preload".
Everyone looking at that section knows what that file does. I also
would omit that header completely if the file cannot be opened.

2141   bool file_present = _print_ascii_file("/etc/ld.so.preload", st);
2142   if (!file_present) { st->print("file not present"); }
2143   st->cr();

You do not know that the file is not preset, all you know is that the
open() call in _print_ascii_file() failed. Since existence of this
file is optional, and not typical, I would just omit messages if it
cannot be found.

Best Regards, Thomas










> Thanks, Thomas
>
>
>
> On Fri, Sep 21, 2018 at 11:13 AM, Baesken, Matthias
> <matthias.baesken at sap.com> wrote:
>> Hello , please review this small patch .
>>
>> It adds more information  about  pre-loading configs  of libs on Linux  (often used for  monitoring tools).
>>
>> Currently the hs_err file contains  already the LD_PRELOAD environment variable.
>> However for system wide configuration, the /etc/ld.so.preload file is important as well on Linux. So display it as well (in case it is present).
>>
>> See the  Environment / LD_PRELOAD   and FILES   sections of :
>>
>> http://man7.org/linux/man-pages/man8/ld.so.8.html
>>
>> for more information .
>>
>>
>>
>> Bug/webrev :
>>
>> https://bugs.openjdk.java.net/browse/JDK-8210964
>>
>> http://cr.openjdk.java.net/~mbaesken/webrevs/8210964.0/
>>
>>
>> Thanks, Matthias