RFR [XS]: 8224958: add os::dll_load calls to event log (original) (raw)

David Holmes david.holmes at oracle.com
Thu Jun 6 07:02:22 UTC 2019

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 errorreport NULL check >      > >      > http://cr.openjdk.java.net/~mbaesken/webrevs/8224958.2/ > >     You missed one in src/hotspot/os/bsd/osbsd.cpp > >     +   if (errorreport == NULL) { >     +     errorreport = "dlerror returned no error description"; >     +   } >     +   if (errorreport && 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. 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 … >      > >      > >      > >      > Best regards, Matthias >      > >      > >      > From: Thomas Stüfe <thomas.stuefe at gmail.com_ _<mailto:thomas.stuefe at gmail.com> >     <mailto: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> >     <mailto: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> >     <mailto:hotspot-dev at openjdk.java.net_ _<mailto:hotspot-dev at openjdk.java.net>> >      > Subject: Re: RFR [XS]: 8224958: add os::dllload calls to event log >      > >      > Hi Matthias, >      > >      > looks good. Minor nit: >      > >      > +  if (errorreport == NULL) { >      > +    errorreport = "dlerror returned no error description"; >      > +  } >      > +  if (errorreport && ebuf && ebuflen > 0) { >      > >      > Since you ensure errorreport 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/oswindows.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 >      > >      > >

More information about the hotspot-dev mailing list