Hi Chris,
This looks okay to me.
Thanks,
Serguei
On 2/11/20 1:49 PM, Chris Plummer wrote:
Hi Igor,
Here's an updated webrev:
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>> 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
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Â seems to
be a webrev from another issue, should it have beenÂ
http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/Â ?
-- Igor
On Jan 30, 2020, at 10:10 PM, Chris Plummer
<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