RFR: 8148188: Enhance the security libraries to record events of interest

[Seán Coffey]

Erik,

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(..)

http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/

regards,
Sean.

On 09/07/2018 18:01, Seán Coffey wrote:
> Erik,
>
> 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 
>> underscore).
> 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.
>
> regards,
> Sean.
>