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

[Baesken, Matthias]

Hello, I would keep  the last revision as it is .
David may I add you as reviewer ?

Best regards, Matthias


> -----Original Message-----
> From: David Holmes <david.holmes at oracle.com>
> Sent: Donnerstag, 6. Juni 2019 09:02
> To: Thomas Stüfe <thomas.stuefe at gmail.com>
> Cc: Baesken, Matthias <matthias.baesken at sap.com>; hotspot-
> dev at openjdk.java.net
> Subject: Re: RFR [XS]: 8224958: add os::dll_load calls to event log
> 
> On 6/06/2019 4:57 pm, Thomas Stüfe wrote:
> > On Thu, Jun 6, 2019 at 1:18 AM David Holmes <david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>> wrote:
> >
> >     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>
> >      > <mailto: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.
> >
> >
> > Lack of coffee :)
> 
> Alas I have tried a range of beverages but none seem to help ;-)
> 
> >     I thought we were logging the text version of the error - why
> >     aren't we?
> >
> >
> > This comes down to taste. I personally prefer a numerical error code
> > (would prefer this on the POSIX platforms too but dlopen() is this weird
> > outlier that does not set errno).
> >
> > I'm fine with both error code or error text. Matthias should decide.
> 
> Okay I'm fine with that.
> 
> >     The error message has to be captured before the log call as logging may
> >     change what is returned by GetLastError.
> >
> > Which is what Matthias did?
> 
> Yes.
> 
> Cheers,
> David
> 
> > Cheers, Thomas
> >
> >     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 …
> >      >      >