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

[Thomas Stüfe]

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