RFR : 8211326 : add OS user related information to hs_err file

David Holmes david.holmes at oracle.com
Thu Nov 15 13:38:12 UTC 2018 

On 15/11/2018 11:20 pm, Baesken, Matthias wrote:
> I'll remove the else - but I think I need a second reviewer, isn’t it ?

I hadn't reviewed the actual printing of the user info but now I have 
had a good look at it. My only quibble is about the treatment of uid_t 
and gid_t as being the same. It would be marginally cleaner to me if the 
cast to unsigned was done upfront on the library call:

unsigned id = (unsigned) ::getuid();
st->print("uid  : %u ", id);
id = (unsigned) ::geteuid();
...
id = (unsigned) ::getgid();
...

Thanks,
David
-----

> Best regards, Matthias
> 
>> -----Original Message-----
>> From: Thomas Stüfe <thomas.stuefe at gmail.com>
>> Sent: Donnerstag, 15. November 2018 14:15
>> To: David Holmes <david.holmes at oracle.com>
>> Cc: Baesken, Matthias <matthias.baesken at sap.com>; Langer, Christoph
>> <christoph.langer at sap.com>; Volker Simonis <volker.simonis at gmail.com>;
>> HotSpot Open Source Developers <hotspot-dev at openjdk.java.net>
>> Subject: Re: RFR : 8211326 : add OS user related information to hs_err file
>>
>> On Thu, Nov 15, 2018 at 1:43 PM David Holmes <david.holmes at oracle.com>
>> wrote:
>>>
>>> On 15/11/2018 10:33 pm, Thomas Stüfe wrote:
>>>> I'm fine with this, Matthias.
>>>>
>>>> BTW, I looked and the STEP header line is only printed when secondary
>>>> exceptions happen inside the step, not always as David assumed (?). So
>>>
>>> Yes my bad - sorry.
>>
>> no problem. I did not see it right away either.
>>
>>>
>>>> your first form
>>>>
>>>> STEP("blabla")
>>>>     if (ExtensiveErrorReports && _verbose) {
>>>>        blub
>>>>     }
>>>>
>>>> would have been fine too I think. But I do not want to send you
>>>> through another review iteration, so I am fine with this version.
>>>
>>> Deleting the else doesn't need another round of review.
>>
>> Same from me. Remove the else path and ship it.
>>
>> ..Thomas
>>
>>>
>>> Thanks,
>>> David
>>>
>>>
>>>> Thanks for incorporating my remarks.
>>>>
>>>> Cheers,Thomas
>>>>
>>>>
>>>> On Thu, Nov 15, 2018 at 12:51 PM Baesken, Matthias
>>>> <matthias.baesken at sap.com> wrote:
>>>>>
>>>>> Hi Thomas,
>>>>>
>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8211326.3/
>>>>>
>>>>>
>>>>> - got  rid of buf+buflen arguments of print_user_info
>>>>> - removed "Removing"
>>>>> - adjusted the output a bit   in  os::Posix::print_user_info()
>>>>> - vmError.cpp  added a short  output line  with info in case   (
>> ExtensiveErrorReports && _verbose  )  is false
>>>>>
>>>>>
>>>>> Best regards, Matthias
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Thomas Stüfe <thomas.stuefe at gmail.com>
>>>>>> Sent: Mittwoch, 14. November 2018 18:43
>>>>>> To: Baesken, Matthias <matthias.baesken at sap.com>
>>>>>> Cc: David Holmes <david.holmes at oracle.com>; Langer, Christoph
>>>>>> <christoph.langer at sap.com>; Volker Simonis
>> <volker.simonis at gmail.com>;
>>>>>> HotSpot Open Source Developers <hotspot-dev at openjdk.java.net>
>>>>>> Subject: Re: RFR : 8211326 : add OS user related information to hs_err
>> file
>>>>>>
>>>>>> Hi Matthias,
>>>>>>
>>>>>> none of my notes from my earlier mail are addressed? I guess my mail
>>>>>> got lost in all that discussion. Here you go:
>>>>>>
>>>>>> +  static void print_user_info(outputStream* st, char* buf, size_t
>> buflen);
>>>>>>
>>>>>> You can get rid of buf+buflen arguments, not needed anymore
>>>>>> ---
>>>>>>
>> http://cr.openjdk.java.net/~mbaesken/webrevs/8211326.2/src/hotspot/os/
>>>>>> posix/os_posix.cpp.udiff.html
>>>>>>
>>>>>> In os::Posix::print_user_info(), for brevity, I would print it all one
>>>>>> line like this:  "uid: 4711, euid: 0, euid: 4711, egid: 4711, umask:
>>>>>> rwxr--r--"
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> The "removing" text does not make sense and can be removed.
>>>>>>
>>>>>> Best Regards, Thomas
>>>>>>
>>>>>>
>>>>>> On Wed, Nov 14, 2018 at 6:16 PM Baesken, Matthias
>>>>>> <matthias.baesken at sap.com> wrote:
>>>>>>>
>>>>>>> Hi Thomas , so are you fine with the latest revision
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8211326.1/
>>>>>>>
>>>>>>> of the patch ,  can I add you as reviewer ?
>>>>>>>
>>>>>>> David - is the info added by Thomas sufficient for you?
>>>>>>>
>>>>>>> Any other reviewers ?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Thomas Stüfe <thomas.stuefe at gmail.com>
>>>>>>>> Sent: Samstag, 10. November 2018 14:04
>>>>>>>> To: David Holmes <david.holmes at oracle.com>
>>>>>>>> Cc: Baesken, Matthias <matthias.baesken at sap.com>; Langer,
>> Christoph
>>>>>>>> <christoph.langer at sap.com>; Volker Simonis
>>>>>> <volker.simonis at gmail.com>;
>>>>>>>> HotSpot Open Source Developers <hotspot-
>> dev at openjdk.java.net>
>>>>>>>> Subject: Re: RFR : 8211326 : add OS user related information to
>> hs_err file
>>>>>>>>
>>>>>>>> Hi David,
>>>>>>>>
>>>>>>>> On Sat, Nov 10, 2018 at 12:59 PM David Holmes
>>>>>> <david.holmes at oracle.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> Hi Matthias,
>>>>>>>>>
>>>>>>>>> On 9/11/2018 3:13 AM, Baesken, Matthias wrote:
>>>>>>>>>> Hello  , after the flag   "-XX:+-ExtensiveErrorReports"   make it into
>>>>>> jdk/jdk
>>>>>>>> , I created another webrev :
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8211326.1/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The user info output is now guarded  by ExtensiveErrorReports .
>>>>>>>>>
>>>>>>>>> +   STEP("printing user info")
>>>>>>>>> +      if (ExtensiveErrorReports) {
>>>>>>>>> +        if (_verbose) {
>>>>>>>>> +          os::Posix::print_user_info(st, buf, sizeof(buf));
>>>>>>>>> +        }
>>>>>>>>> +      }
>>>>>>>>>
>>>>>>>>> I don't understand why we explicitly need _verbose if we've asked
>> for
>>>>>>>>> ExtensiveErrorreports?
>>>>>>>>
>>>>>>>> That flag has a different purpose: _verbose distinguishes the two
>>>>>>>> calls to VMError::report(): the first one to print a small report to
>>>>>>>> stderr, the second print a large report to the hs-err file. We only
>>>>>>>> want to print to the hs-err file, so _verbose is still needed.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Also, ideally the STEP would be inside the guard as otherwise we
>> will
>>>>>>>>> just print the step description followed by nothing. If the macro
>>>>>>>>> expansion makes that impossible then we should have an else
>> clause
>>>>>> that
>>>>>>>>> prints something like:
>>>>>>>>>
>>>>>>>>>     - disabled (use -XX:+ExtensiveErrorReports to see these details)
>>>>>>>>>
>>>>>>>>
>>>>>>>> We may even go a step further (but in a separate patch) and make
>>>>>>>> "ExtensiveErrorReports" a property of the STEP, to be given as
>>>>>>>> parameter of the STEP macro.
>>>>>>>>
>>>>>>>> Thanks, Thomas
>>>>>>>>
>>>>>>>

More information about the hotspot-dev mailing list