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

[Baesken, Matthias]

Hi Thomas , I removed  the error_report _ NULL check

http://cr.openjdk.java.net/~mbaesken/webrevs/8224958.2/


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


  *   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