|
Hi Igor,
I'm not sure what you mean by restore the interrupt state. Do you
mean loop back to the waitFor() call?
thanks,
Chris
On 2/11/20 1:55 PM, Igor Ignatyev wrote:
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
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
|
