<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">Hi Jini,<div><br></div><div>It looks good to me but I have a few nits:</div><div><br></div><div><a href="http://cr.openjdk.java.net/~jgeorge/8215544/webrev.03/test/hotspot/jtreg/serviceability/sa/ClhsdbLauncher.java.udiff.html">http://cr.openjdk.java.net/~jgeorge/8215544/webrev.03/test/hotspot/jtreg/serviceability/sa/ClhsdbLauncher.java.udiff.html</a><br></div><div>  +  Â  Â  Â  Â  Â  Â  Â  // by appending 'sudo' to it. Check now if sudo passes in this</div><div>  +  Â  Â  Â  Â  Â  Â  Â  // environment with a simple 'echo' command.</div><div>  Â  Â -> 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</div><div><br></div><div><a href="http://cr.openjdk.java.net/~jgeorge/8215544/webrev.03/test/lib/jdk/test/lib/SA/SATestUtils.java.html">http://cr.openjdk.java.net/~jgeorge/8215544/webrev.03/test/lib/jdk/test/lib/SA/SATestUtils.java.html</a>  <br></div><div>  Â  -> You put some System.out.println; are they intended to still be there or were they for debug?</div><div><br></div><div><pre style="color:rgb(0,0,0)"><br class="gmail-Apple-interchange-newline">  77         for (String val: cmdStringList) {
  78             outStringList.add(val);
  79         }</pre><pre style="color:rgb(0,0,0)"><br></pre><pre style="color:rgb(0,0,0)">Couldn't we use addAll ?</pre><pre style="color:rgb(0,0,0)">Thanks,</pre><pre style="color:rgb(0,0,0)">Jc</pre></div></div></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Jan 7, 2019 at 8:33 AM Jini George <<a href="mailto:jini.george@oracle.com">jini.george@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Gentle reminder !<br>
<br>
Thanks,<br>
Jini.<br>
<br>
On 1/3/2019 11:46 PM, Jini George wrote:<br>
> Hello!<br>
> <br>
> Requesting reviews for:<br>
> <br>
> <a href="https://bugs.openjdk.java.net/browse/JDK-8215544" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8215544</a><br>
> Webrev: <a href="http://cr.openjdk.java.net/~jgeorge/8215544/webrev.03/index.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jgeorge/8215544/webrev.03/index.html</a><br>
> <br>
> The changes here are primarily for modifying the ClhsdbLauncher used for <br>
> SA tests to check if 'sudo' privileges can be added for executing MacOS <br>
> tests, and if so, adding these privileges before executing the tests. <br>
> This is related to the changes which were proposed with:<br>
> <br>
> <a href="http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-July/024373.html" rel="noreferrer" target="_blank">http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-July/024373.html</a> <br>
> <br>
> <br>
> for (JDK-8199700: SA: Enable jhsdb jtreg tests for Mac OS X).<br>
> <br>
> The issue brought out with the proposed changes for JDK-8199700 have <br>
> been fixed. This patch addresses the case where 'sudo' fails with errors <br>
> like:<br>
> <br>
> sudo: no tty present and no askpass program specified<br>
>  Â or<br>
> sudo: a password is required<br>
> <br>
> or if 'sudo' waits for a password. In these cases, the tests continue to <br>
> get skipped.<br>
> <br>
> Many thanks to Goetz Lindenmaier for helping me with multiple rounds of <br>
> testing of this in the SAP test farms! With this patch, about 22 SA <br>
> tests get enabled now on MacOS on Mach5.<br>
> <br>
> More details at: <a href="https://bugs.openjdk.java.net/browse/JDK-8215544" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8215544</a><br>
> <br>
> Thanks,<br>
> Jini.<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>