RFR: 8148188: Enhance the security libraries to record events of interest
sean.coffey at oracle.com
Wed Oct 17 10:25:57 UTC 2018
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.
Some of the main changes worthy of highlighting :
* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEvent (old event name :
* Introduce CertificateSerialNumber type to make use of the @Relational
* Keep calls for JFR event operations local to call site to optimize jvm
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/
This code is dependent on the JDK-8203629 enhancement being integrated.
On 10/07/18 13:50, Seán Coffey wrote:
> 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.
> 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.
> 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.
> 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.
> I've removed the variable names using underscore. Also optimized some
> variable assignments in X509Impl.commitEvent(..)
> On 09/07/2018 18:01, Seán Coffey wrote:
>> Thanks for reviewing. Comments inline..
>> On 09/07/18 17:21, Erik Gahlin wrote:
>>> Thanks Sean.
>>> Some feedback on the code in the security libraries.
>>> - We should use camel case naming convention for variables (not
>> Sure. I see two offending variable names which I'll fix up.
>>> - Looking at sun/security/ssl/Finished.java,
>>> 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.
>> 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.
More information about the core-libs-dev