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

[Daniel D. Daugherty]

On 6/5/19 8:50 AM, David Holmes 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) {

Ummm... Why check error_report in the second if-statement?
Because of the preceding if-statement it can never be NULL.

Also the implied booleans need to be fixed:

     s/ebuf/ebuf != NULL/

I haven't reviewed the whole thing. This just jumped out at
me as I was reading the thread...

Dan


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