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

[David Holmes]

On 5/06/2019 10:58 pm, Thomas Stüfe wrote:
> On Wed, Jun 5, 2019 at 2:50 PM David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> 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) {
> 
>      >
>      >    *   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.
> 
> 
> not quite :)
> 
> Matthias prints out the numerical errcode in his call to Events::log, 
> retrieved by GetLastError() some lines above. That has nothing to do 
> with lasterror and errbuf. lasterror() is used to fill errbuf. It too 
> calls GetLastError(), which makes it position dependend. Matthias added 
> a new call into the EventLog system, so he is careful and moves the call 
> to lasterror() up in case the EventLog system calls system APIs and 
> overwrites the error code.

<sigh> Sometimes my brain continues to see what it thinks it should be 
seeing. I thought we were logging the text version of the error - why 
aren't we?

The error message has to be captured before the log call as logging may 
change what is returned by GetLastError.

David

> 
> ..Thomas
> 
> 
>     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
>     <mailto:thomas.stuefe at gmail.com>>
>      > Sent: Mittwoch, 5. Juni 2019 09:31
>      > To: Baesken, Matthias <matthias.baesken at sap.com
>     <mailto:matthias.baesken at sap.com>>
>      > Cc: hotspot-dev at openjdk.java.net
>     <mailto: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
>      >
>      >
>