Thank you very much, Paul for the review. Yes, this is only needed for OSX.
Thanks, Jini. On 1/12/2019 5:41 AM, Hohensee, Paul wrote:
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
