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

[Baesken, Matthias]

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