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

Thu Nov 15 13:20:58 UTC 2018

I'll remove the else - but I think I need a second reviewer, isn’t it ?

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