RFR [XS]: 8224958: add os::dll_load calls to event log

[Doerr, Martin]

Hi Matthias,

looks good.

Best regards,
Martin


> -----Original Message-----
> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of
> Baesken, Matthias
> Sent: Mittwoch, 5. Juni 2019 17:06
> To: daniel.daugherty at oracle.com; David Holmes
> <david.holmes at oracle.com>; Thomas Stüfe <thomas.stuefe at gmail.com>
> Cc: hotspot-dev at openjdk.java.net
> Subject: RE: RFR [XS]: 8224958: add os::dll_load calls to event log
> 
> Hello, here is an updated webrev  with adjusted NULL checks  :
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8224958.3/
> 
> Best regards, Matthias
> 
> 
> > -----Original Message-----
> > From: Daniel D. Daugherty <daniel.daugherty at oracle.com>
> > Sent: Mittwoch, 5. Juni 2019 16:43
> > To: David Holmes <david.holmes at oracle.com>; Baesken, Matthias
> > <matthias.baesken at sap.com>; Thomas Stüfe
> <thomas.stuefe at gmail.com>
> > Cc: hotspot-dev at openjdk.java.net
> > Subject: Re: RFR [XS]: 8224958: add os::dll_load calls to event log
> >
> > On 6/5/19 8:50 AM, David Holmes wrote:
> > > On 5/06/2019 7:06 pm, Baesken, Matthias wrote:
> > >> Hi Thomas , I removed  the error_report _ NULL check
> > >>
> > >> http://cr.openjdk.java.net/~mbaesken/webrevs/8224958.2/
> > >
> > > You missed one in src/hotspot/os/bsd/os_bsd.cpp
> > >
> > > +   if (error_report == NULL) {
> > > +     error_report = "dlerror returned no error description";
> > > +   }
> > > +   if (error_report && ebuf && ebuflen > 0) {
> >
> > Ummm... Why check error_report in the second if-statement?
> > Because of the preceding if-statement it can never be NULL.
> >
> > Also the implied booleans need to be fixed:
> >
> >      s/ebuf/ebuf != NULL/
> >
> > I haven't reviewed the whole thing. This just jumped out at
> > me as I was reading the thread...
> >
> > Dan
> >
> >
> > >
> > >>
> > >>    *   You moved lasterror() up to get a "fresh" error value before
> > >> calling into event logging right? I needed a while to understand
> > >> that. Okay.
> > >>
> > >> Yes this was my intention .
> > >
> > > Not sure I'd say "fresh" is the right description. An error has
> > > occurred and you want the error message for log statement. Its the
> > > same error message you'd get later as no other system call is being
> > > made. There's no difference to the how the code currently works, we
> > > just want the error text earlier for the log.
> > >
> > > David
> > > -----
> > >
> > >>
> > >>
> > >>    *   It would be better to have a function which only does the text
> > >> translation, using an error code we give it as parameter.
> > >>    *   Then we could have given it the errcode we already have. Would
> > >> be good for a future improvement.
> > >>
> > >> There are not so many  os::lasterror   usages currently .  So I am
> > >> not sure if it is really needed ,   let’s  do it in a separate CR
> > >> any way …
> > >>
> > >>
> > >>
> > >> Best regards, Matthias
> > >>
> > >>
> > >> From: Thomas Stüfe <thomas.stuefe at gmail.com>
> > >> Sent: Mittwoch, 5. Juni 2019 09:31
> > >> To: Baesken, Matthias <matthias.baesken at sap.com>
> > >> Cc: hotspot-dev at openjdk.java.net
> > >> Subject: Re: RFR [XS]: 8224958: add os::dll_load calls to event log
> > >>
> > >> Hi Matthias,
> > >>
> > >> looks good. Minor nit:
> > >>
> > >> +  if (error_report == NULL) {
> > >> +    error_report = "dlerror returned no error description";
> > >> +  }
> > >> +  if (error_report && ebuf && ebuflen > 0) {
> > >>
> > >> Since you ensure error_report is always set - with a default text, if
> > >> needed - you do not need to check it for NULL afterwards.
> > >>
> > >> If you only fix this, I do not need a new webrev.
> > >>
> > >> ---
> > >>
> > >> Side note:
> > >>
> > >>
> >
> http://cr.openjdk.java.net/~mbaesken/webrevs/8224958.1/src/hotspot/os/
> > windows/os_windows.cpp.udiff.html
> > >>
> > >>
> > >> You moved lasterror() up to get a "fresh" error value before calling
> > >> into event logging right? I needed a while to understand that. Okay.
> > >>
> > >> This shows that lasterror() is actually not well designed.
> > >> lasterror() does two things: 1) get the error code with GetLastError
> > >> and 2) translate that numerical code to a text.
> > >>
> > >> It would be better to have a function which only does the text
> > >> translation, using an error code we give it as parameter. Then we
> > >> could have given it the errcode we already have. Would be good for a
> > >> future improvement.
> > >> Cheers, Thomas
> > >>
> > >>