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

[Baesken, Matthias]

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