A more generic approach that doesn’t check for a specific platform in
ClhsdbLauncher.java would be nice, e.g., add a checkForPrivileges() method to
Platform, but if OSX is the only platform that needs the check, I'm ok with
this patch.
Thanks,
Paul
On 1/10/19, 8:16 PM, "serviceability-dev on behalf of Jini George"
<serviceability-dev-boun...@openjdk.java.net on behalf of
jini.geo...@oracle.com> wrote:
Thanks again, JC! :-)
Folks, Could I please get one more review on this ?
-Jini.
On 1/11/2019 9:38 AM, JC Beyler wrote:
> Hi Jini,
>
> Looks good to me now :) Thanks for handling the comments :)
> Jc
>
>
> On Tue, Jan 8, 2019 at 8:11 PM Jini George <jini.geo...@oracle.com
> <mailto:jini.geo...@oracle.com>> wrote:
>
> Thank you very much for the review, JC. My comments inline:
>
> On 1/8/2019 12:22 AM, JC Beyler wrote:
> >
> >
>
http://cr.openjdk.java.net/~jgeorge/8215544/webrev.03/test/hotspot/jtreg/serviceability/sa/ClhsdbLauncher.java.udiff.html
> > + // by appending 'sudo' to it. Check now if sudo
> > passes in this
> > + // environment with a simple 'echo' command.
> > -> Really shouldn't provide implementation details about
> what is
> > being done by that method; if we change that, this comment will
> rot :-)
> > canAddPrivileges is explicit enough in my mind
>
> Done. I have removed the comment.
>
> >
> >
>
http://cr.openjdk.java.net/~jgeorge/8215544/webrev.03/test/lib/jdk/test/lib/SA/SATestUtils.java.html
>
> >
> > -> You put some System.out.println; are they intended to
> still be
> > there or were they for debug?
>
> I guess you meant this one:
> System.out.println("Adding 'sudo -E' to the command.");
>
> This one is meant to be there.
>
> >
> >
> > 77 for (String val: cmdStringList) {
> > 78 outStringList.add(val);
> > 79 }
> >
> >
> > Couldn't we use addAll ?
>
> Done.
> The modified webrev is at:
>
> http://cr.openjdk.java.net/~jgeorge/8215544/webrev.04/index.html
>
> Thanks,
> Jini.
>
>
>
>
>
> --
>
> Thanks,
> Jc
