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

[coleen.phillimore at oracle.com]

This version .1 seems fine.  This isn't a very long file, is it? People 
post the whole hs_err_pid file in bug descriptions which makes for a lot 
of scrolling.
Looks good to me.
Coleen


On 9/26/18 9:14 AM, Langer, Christoph wrote:
> Hi Matthias,
>
> just checked the change. I like the second variant (8210964.1) better. Reviewed.
>
> Best regards
> Christoph
>
>> -----Original Message-----
>> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of
>> Baesken, Matthias
>> Sent: Dienstag, 25. September 2018 09:14
>> To: Thomas Stüfe <thomas.stuefe at gmail.com>
>> Cc: hotspot-dev at openjdk.java.net
>> Subject: RE: RFR [XS] 8210964: add more ld preloading info to hs_error file on
>> Linux
>>
>> Thanks,  can I have a second review please  ?
>>
>> (maybe with opinions  what people like better,
>>
>> http://cr.openjdk.java.net/~mbaesken/webrevs/8210964.0/
>>
>> or
>>
>> http://cr.openjdk.java.net/~mbaesken/webrevs/8210964.1/
>>
>> )
>>
>> Best regards, Matthias
>>
>>
>>> -----Original Message-----
>>> From: Thomas Stüfe <thomas.stuefe at gmail.com>
>>> Sent: Montag, 24. September 2018 16:45
>>> To: Baesken, Matthias <matthias.baesken at sap.com>
>>> Cc: hotspot-dev at openjdk.java.net
>>> Subject: Re: RFR [XS] 8210964: add more ld preloading info to hs_error file
>> on
>>> Linux
>>>
>>> Hi Matthias,
>>>
>>> Hm. I see now that my request to not print the header line of
>>> ld.preload made the patch more complicated than necessary. Sorry for
>>> that.
>>>
>>> If you like, you can revert to the previous version, I would just ask
>>> you to change the error message to a more generic sentence, e.g.:
>>>
>>> void os::Linux::print_system_config_info(outputStream* st) {
>>>    st->cr();
>>>    st->print_cr("/etc/ld.so.preload (system-wide LD_PRELOAD config file):");
>>>    bool success = _print_ascii_file("/etc/ld.so.preload", st);
>>>    if (!success) { st->print("not available"); }
>>>    st->cr();
>>>    st->cr();
>>> }
>>>
>>> I am fine with both versions. I leave it up to you if you ship the
>>> second variant of the first one (with the changed error message).
>>>
>>> I do not need another webrev.
>>>
>>> Thanks, Thomas
>>>
>>>
>>>
>>> On Mon, Sep 24, 2018 at 3:14 PM, Baesken, Matthias
>>> <matthias.baesken at sap.com> wrote:
>>>> Hi Thomas,  thanks for your input !
>>>>
>>>> I followed your suggestions :
>>>> - renamed  the function to os::Linux::print_ld_preload_file
>>>> - output  the header only in case the file is opened
>>>> - changed (shortened) the header
>>>>
>>>>>> Also, I personally would not expose it via os_linux.hpp.
>>>> However I kept it there,  to keep it in line with the other info-functions.
>>>>
>>>> New webrev :
>>>>
>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8210964.1/
>>>>
>>>>
>>>> Best regards, Matthias
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Thomas Stüfe <thomas.stuefe at gmail.com>
>>>>> Sent: Freitag, 21. September 2018 18:59
>>>>> To: Baesken, Matthias <matthias.baesken at sap.com>
>>>>> Cc: hotspot-dev at openjdk.java.net
>>>>> Subject: Re: RFR [XS] 8210964: add more ld preloading info to hs_error
>> file
>>> on
>>>>> Linux
>>>>>
>>>>> 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
>>>>>