<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>With JDK-8203629 now pushed, I've re-based my patch on latest
jdk/jdk code.</p>
<p>Some modifications also made based on off-thread suggestions : <br>
</p>
<p>src/java.base/share/classes/java/security/Security.java<br>
</p>
<p>* Only record JDK security properties for now (i.e. those in
java.security conf file) <br>
- we can consider a new event to record non-JDK security events
if demand comes about<br>
- new event renamed to <b>Jdk</b>SecurityPropertyModificationEvent</p>
<p>src/java.base/share/classes/sun/security/x509/X509CertImpl.java<br>
</p>
<p>* Use hashCode() equality test for storing certs in List. <br>
</p>
<p>Tests updated to capture these changes</p>
<p>src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java</p>
<p>* Verify that self signed certs exercise the modified code paths</p>
<p>I've added new test functionality to test use of self signed
cert.</p>
<p>webrev :
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~coffeys/webrev.8148188.v7/webrev/">http://cr.openjdk.java.net/~coffeys/webrev.8148188.v7/webrev/</a><br>
</p>
<pre class="moz-signature" cols="72">Regards,
Sean.</pre>
<div class="moz-cite-prefix">On 17/10/18 11:25, Seán Coffey wrote:<br>
</div>
<blockquote
cite="mid:99c29269-1332-1a2d-c2a2-4b7479177488@oracle.com"
type="cite">I'd like to re-boot this review. I've been working
with Erik off thread who's been helping with code and test design.
<br>
<br>
Some of the main changes worthy of highlighting : <br>
<br>
* Separate out the tests for JFR and Logger events <br>
* Rename some events <br>
* Normalize the data view for X509ValidationEvent (old event name
: CertChainEvent) <br>
* Introduce CertificateSerialNumber type to make use of the
@Relational JFR annotation. <br>
* Keep calls for JFR event operations local to call site to
optimize jvm compilation <br>
<br>
webrev : <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecoffeys/webrev.8148188.v6/webrev/">http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/</a>
<br>
<br>
This code is dependent on the JDK-8203629 enhancement being
integrated. <br>
<br>
Regards, <br>
Sean. <br>
<br>
On 10/07/18 13:50, Seán Coffey wrote: <br>
<blockquote type="cite">Erik, <br>
<br>
After some trial edits, I'm not so sure if moving the event
& logger commit code into the class where it's used works
too well after all. <br>
<br>
In the code you suggested, there's an assumption that calls such
as EventHelper.certificateChain(..) are low cost. While that
might be the case here, it's something we can't always assume.
It's called once for the JFR operation and once for the Logger
operation. That pattern multiplies itself further in other
Events. i.e. set up and assign the variables for a JFR event
without knowing whether they'll be needed again for the Logger
call. We could work around it by setting up some local variables
and re-using them in the Logger code but then, we're back to
where we were. The current EventHelper design eliminates this
problem and also helps to abstract the recording/logging
functionality away from the functionality of the class itself. <br>
<br>
It also becomes a problem where we record events in multiple
different classes (e.g. SecurityPropertyEvent). While we could
leave the code in EventHelper for this case, we then have a
mixed design pattern. <br>
<br>
Are you ok with leaving as is for now? A future wish might be
one where JFR would handle Logger via its own framework/API in a
future JDK release. <br>
<br>
I've removed the variable names using underscore. Also optimized
some variable assignments in X509Impl.commitEvent(..) <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecoffeys/webrev.8148188.v5/webrev/">http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/</a>
<br>
<br>
regards, <br>
Sean. <br>
<br>
On 09/07/2018 18:01, Seán Coffey wrote: <br>
<blockquote type="cite">Erik, <br>
<br>
Thanks for reviewing. Comments inline.. <br>
<br>
On 09/07/18 17:21, Erik Gahlin wrote: <br>
<blockquote type="cite">Thanks Sean. <br>
<br>
Some feedback on the code in the security libraries. <br>
<br>
- We should use camel case naming convention for variables
(not underscore). <br>
</blockquote>
Sure. I see two offending variable names which I'll fix up. <br>
<blockquote type="cite"> <br>
- Looking at sun/security/ssl/Finished.java, <br>
<br>
I wonder if it wouldn't be less code and more easy to read,
if we would commit the event in a local private function and
then use the EventHelper class for common functionality. <br>
</blockquote>
I'm fine with your suggested approach. I figured that tucking
the recording/logging logic away in a helper class also had
benefits but I'll re-factor to your suggested style unless I
hear objections. <br>
<br>
regards, <br>
Sean. <br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>