RFR: 8234741: enhance os::get_core_path on macOS

Baesken, Matthias matthias.baesken at sap.com
Thu Nov 28 11:09:37 UTC 2019


Thanks for the review.

> Please also add os:: to the other call to current_process_id() in line 3787

Okay.

Gerard, may I add you as reviewer ?


Best regards, Matthias



> -----Original Message-----
> From: Langer, Christoph <christoph.langer at sap.com>
> Sent: Donnerstag, 28. November 2019 12:03
> To: Baesken, Matthias <matthias.baesken at sap.com>; gerard ziemski
> <gerard.ziemski at oracle.com>
> Cc: hotspot-dev developers <hotspot-dev at openjdk.java.net>
> Subject: RE: RFR: 8234741: enhance os::get_core_path on macOS
> 
> Hi Matthias,
> 
> this looks good to me.
> 
> Please also add os:: to the other call to current_process_id() in line 3787 (the
> else branch). No need for another webrev, though.
> 
> /Christoph
> 
> > -----Original Message-----
> > From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf
> Of
> > Baesken, Matthias
> > Sent: Mittwoch, 27. November 2019 11:09
> > To: gerard ziemski <gerard.ziemski at oracle.com>
> > Cc: hotspot-dev developers <hotspot-dev at openjdk.java.net>
> > Subject: RE: RFR: 8234741: enhance os::get_core_path on macOS
> >
> > Hi Gerard, thanks for your input .
> >
> > New webrev :
> >
> > http://cr.openjdk.java.net/~mbaesken/webrevs/8234741.1/
> >
> >
> > Best regards, Matthias
> >
> >
> > >
> > > hi Matthias,
> > >
> > > I had to look up "kern.corefile" option, of which I was not previously
> > > aware - it looks like a nice enhancement.
> > >
> > > I'd like to suggest a few small cleanups though:
> > >
> > > #1 add "os::" prefix to "current_process_id()" call
> > > #2 restrict the scope of "os::current_process_id()" to only the branch
> > > of "if" that needs it
> > > #3 expand on the "tail" comment a bit to explain why it might be needed
> > >
> > > Perhaps something like this:
> > >
> > >    char coreinfo[MAX_PATH];
> > >     size_t sz = sizeof(coreinfo);
> > >     int ret = sysctlbyname("kern.corefile", coreinfo, &sz, NULL, 0);
> > >     if (ret == 0) {
> > >       char *pid_pos = strstr(coreinfo, "%P");
> > >       const char* tail = (pid_pos != NULL) ? (pid_pos + 2) : ""; // skip
> > > over the "%P" to preserve any optional custom user pattern (i.e. %N,
> %U)
> > >       if (pid_pos != NULL) {
> > >         *pid_pos = '\0';
> > >         n = jio_snprintf(buffer, bufferSize, "%s%d%s", coreinfo,
> > > os::current_process_id(), tail);
> > >       } else {
> > >         n = jio_snprintf(buffer, bufferSize, "%s", coreinfo);
> > >       }
> > >     }
> > >
> > > BTW. I'm glad you agree to remove the unrelated AWT change from this
> fix
> > > and let the client team handle it.
> > >
> > >



More information about the hotspot-dev mailing list