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

[Langer, Christoph]

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