Hi Chris, I don't insist on (3), so I'm fine if you don't want to change that part. one thing I'd change though is to restore thread interrupted state at L#266 of Platform.java (no need to publish new webrev)
Thanks, -- Igor > On Feb 11, 2020, at 1:49 PM, Chris Plummer <chris.plum...@oracle.com> wrote: > > Hi Igor, > > Here's an updated webrev: > > http://cr.openjdk.java.net/~cjplummer/8238196/webrev.01/index.html > <http://cr.openjdk.java.net/~cjplummer/8238196/webrev.01/index.html> > > I rebased to JDK 15 and made all the changes you suggested except for (3). I > did not think it is necessary since the code is only executed on OSX. > However, if you still feel allowing flexibility in the path separator is > important, I can add that change too. > > thanks, > > Chris > > On 2/10/20 1:34 PM, Igor Ignatyev wrote: >> Hi Chris, >> >> in general it all looks good, I have a few comments (most of them are >> editorial): >> in Platform.java: >> 1. you have doubled spaced at line#238 (b/w boolean and isSignedOSX) >> 2. as FileNotFoundException is IOException, there is no need to declare the >> former in the signature of isSignedOSX >> 3. it's better to pass jdkPath, "bin" and "java" as separate arguments to >> Path.get, so the code won't depend on file separator >> 4. you are waiting for codesign to finish w/o reading its cout / cerr, which >> might lead to a deadlock (if codesign will exhaust IO buffer before >> exiting), so you need to either create two separate threads to read cout and >> cerr or redirect these streams them to files and read these files >> afterwards or just ignore cout/cerr by using Redirect.DISCARD. I'd >> personally recommend the latter as the result of codesign can be reliably >> deduced from its exitcode (0 - signed, 1 - verification failed, 2 - wrong >> arguments, 3 - not all requirements from R are satisfied) and using >> cout/cerr is somewhat fragile as there is no guarantee output format won't >> be changed. >> >> the rest looks good to me. >> >> -- Igor >> >>> On Feb 10, 2020, at 11:48 AM, Chris Plummer <chris.plum...@oracle.com >>> <mailto:chris.plum...@oracle.com><mailto:chris.plum...@oracle.com >>> <mailto:chris.plum...@oracle.com>>> wrote: >>> >>> Ping #2. It's not that hard of a review. Most of it is the new >>> Platform.isSignedOSX() method, which is well commented and pretty straight >>> froward. >>> >>> thanks, >>> >>> Chris >>> >>> On 2/4/20 5:04 PM, Chris Plummer wrote: >>>> Ping! >>>> >>>> And I decided to push to 15 instead of 14. Will backport to 14 eventually. >>>> >>>> thanks, >>>> >>>> Chris >>>> >>>> On 1/30/20 10:20 PM, Chris Plummer wrote: >>>>> Yes, you are correct: >>>>> >>>>> https://bugs.openjdk.java.net/browse/JDK-8238196 >>>>> <https://bugs.openjdk.java.net/browse/JDK-8238196> >>>>> http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00 >>>>> <http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00> >>>>> >>>>> thanks, >>>>> >>>>> Chris >>>>> >>>>> On 1/30/20 10:13 PM, Igor Ignatyev wrote: >>>>>> Hi Chris, >>>>>> >>>>>> http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00 >>>>>> <http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00%C3%82> seems to >>>>>> be a webrev from another issue, should it have been >>>>>> http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/ >>>>>> <http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/%C3%82> ? >>>>>> >>>>>> -- Igor >>>>>> >>>>>>> On Jan 30, 2020, at 10:10 PM, Chris Plummer <chris.plum...@oracle.com >>>>>>> <mailto:chris.plum...@oracle.com><mailto:chris.plum...@oracle.com >>>>>>> <mailto:chris.plum...@oracle.com>>> wrote: >>>>>>> >>>>>>> Hello, >>>>>>> >>>>>>> Please review the following fix for some SA tests that are failing on >>>>>>> Mac OS X 10.14.5 and later: >>>>>>> >>>>>>> https://bugs.openjdk.java.net/browse/JDK-8238196 >>>>>>> http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00 >>>>>>> >>>>>>> The issue is that SA can't attach to a signed binary starting with >>>>>>> 10.14.5. There is no workaround for this, so these tests are being >>>>>>> disabled when it is detected that the binary is signed and we are >>>>>>> running on 10.14 or later (I chose all 10.14 releases to simplify the >>>>>>> check). >>>>>>> >>>>>>> Some background may help explain the fix. In order for SA to attach to >>>>>>> a live process (not a core file) on OSX, either the attaching process >>>>>>> (ie. the test) has to be run as root, or sudo needs to be supported. >>>>>>> However, the only tests that make the sudo check are the 20 or so that >>>>>>> use ClhsdbLauncher. The rest all rely on "@requires >>>>>>> vm.hasSAandCanAttach" to filter out tests that use SA attach. >>>>>>> vm.hasSAandCanAttach only checks if the test is being run as root. Thus >>>>>>> all our non-ClhsdbLauncher tests that SA attach to a live process are >>>>>>> currently not run unless they are run as root. 8238268 [1] has been >>>>>>> filed to address this, making it so all the tests will attempt to use >>>>>>> sudo if not run as root. >>>>>>> >>>>>>> Because of the difference in how ClhsdbLauncher tests and "@requires >>>>>>> vm.hasSAandCanAttach" tests check to see if they are runnable, this fix >>>>>>> needs to address both types of checks. The common code for both these >>>>>>> cases is Platform.shouldSAAttach(), which on OSX basically equates to >>>>>>> check to see if we are running as root. I changed it to also return >>>>>>> false if running on signed binary with 10.14 or later. However, this >>>>>>> confused the ClhsdbLauncher use of Platform.shouldSAAttach() somewhat, >>>>>>> since it assumed a false result only happens because you are not >>>>>>> running as root (in which case it would then check if sudo will work). >>>>>>> So ClhsdbLauncher now has double check that the false result was not >>>>>>> because of running a signed binary. If it is signed, it won't do the >>>>>>> sudo check. This will get cleaned up with 8238268 [1], which will move >>>>>>> the sudo check into Platform.shouldSAAttach(). >>>>>>> >>>>>>> thanks, >>>>>>> >>>>>>> Chris >>>>>>> >>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8238268
