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