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

[Thomas Stüfe]

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


On Wed, Jun 5, 2019 at 9:10 AM Baesken, Matthias <matthias.baesken at sap.com>
wrote:

> Hello  here is  the  new  webrev  ,   it handles now  dlerror  returning
> NULL  (in os::dll_load  )  :
>
>
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8224958.1/
>
>
>
>
>
> Best regards, Matthias
>
>
>
>
>
> *From:* Thomas Stüfe <thomas.stuefe at gmail.com>
> *Sent:* Dienstag, 4. Juni 2019 14:36
> *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
>
>
>
>
>
>
>
> On Tue, Jun 4, 2019 at 2:24 PM Baesken, Matthias <matthias.baesken at sap.com>
> wrote:
>
> Hi Thomas thanks for the input !
>
>
>
>    - . dlerror() may return NULL
>
>
>
> Sure I can add this , makes sense to check for NULL !
>
>
>
>
>
> Looks like there are a few other   places   of dlerror usage where a NULL
> check is missing , one example  :
>
>
>
> jdk/src/jdk.crypto.cryptoki/unix/native/libj2pkcs11/p11_md.c
>
>
>
> 108        systemErrorMessage = dlerror();
>
> 109        exceptionMessage = (char *) malloc(sizeof(char) *
> (strlen(systemErrorMessage) + strlen(libraryNameStr) + 1));
>
>
>
>
>
>
>
>    - add the dlerror output to the log (where easily available)
>
>
>
> My “fear”  was the messages might get a bit too long and eat up too much
> space ,  do you think it is fine ?
>
>
>
>
>
> I think they are rather short. They are static strings baked into the
> libc. You could manually truncate them via %.xxxs, e.g. to 64 chars with
> %.64s . You also could just print to the log, since it will automatically
> truncate at 255 or 512 chars (I forgot which).
>
>
>
> Cheers, Thomas
>
>
>