> On 29 Jun 2018, at 17:34, Seán Coffey wrote:
>
> I've introduced a new test helper class in the jdk/test/lib/jfr directory to
> help with the dual test nature of the new tests. It's helped alot with test
> code duplication.
>
My thinking was to put things like the certificates in a separate file, i.e
TestCertificates, and then have a logging test and a JFR test reuse it.
One rationale for adding logging was to use it if JFR is not present. By
putting the tests together, it becomes impossible to compile and test logging
without having JFR.
> Looked at use of @DataAmount(DataAmount.BITS) also. Not sure if it's fits.
> The output is displayed in units like "KiB" - not the normal when examining
> key lengths used in X509Certificates. i.e a 2048 bit key gets displayed as "2
> KiB" - I'd prefer to keep the 2048 display version.
We should not let the JMC GUI decide how units are specified. There will be
other GUIs and this is the first event that uses bits, so I don’t think it is
formatted that way because it was considered superior.
Erik
>
> new webrev at: http://cr.openjdk.java.net/~coffeys/webrev.8148188.v4/webrev/
>
> Regards,
> Sean.
>
> On 28/06/18 17:59, Seán Coffey wrote:
>> Comments inline.
>>
>>
>> On 28/06/2018 17:20, Erik Gahlin wrote:
>>> It's sufficient if an event object escapes to another method (regardless if
>>> JFR is enabled or not).
>>>
>>> Some more feedback:
>>>
>>> Rename event jdk.CertChain to jdk.CertificateChain
>>> Rename event jdk.X509Cert to jdk.X509Certificate
>>> Rename field certChain to certificateChain.
>>> Rename field serialNum to serialNumber
>> all above done.
>>> Rename field algId to AlgorithmicId or AlgorithmicID
>> maybe "algorithm" works here also ?
>>> Rename @Label("Ciphersuite") to @Label("Cipher Suite")
>>> Rename @Label("Cert Chain") to @Label("Certificate Chain")
>>> Rename @Label("Property Name") to "Name" or "Key" if that makes sense in
>>> the context?
>>> Rename @Label("Property Value") to "Value".
>>> Put events in a subcategory, i.e @Category({"Java Development Kit",
>>> "Security"})
>> done.
>>> Make classes final.
>> done. I had thought that the JFR mechanism required non-final classes.
>>> What is the unit of the key in X509Certificate event? Bits? If that is the
>>> case, use @DataAmount(DataAmount.BITS)
>> Yes - it's essentially the bit length of the keys used. Let me look into
>> that annotation usage.
>>> @Label("Serial numbers forming chain of trust") should not be a sentence.
>>> How about "Serial Numbers"?
>>>
>>> I think the tests are hard to read when two things are tested at the same
>>> time. It is also likely that if a test fail due to logging issues, it will
>>> be assigned to JFR because of the test name, even thought the issues is not
>>> JFR related.
>> I think we're always going to have some ownership issues when tests serve a
>> dual purpose. I still think it makes sense to keep the test logic in one
>> place. Any failures in these tests will most likely be owned by security
>> team. (moving the tests to security directory is also an option)
>>>
>>> If you want to reuse code between tests, I would put it in testlibrary.
>> Let me check if there's any common patterns that could be added to the
>> testlibary.
>>
>> Thanks,
>> Sean.
>>>
>>> Erik
>>>
Thanks for the update Erik. By default I'm proposing that the new JFR
Events and Logger be disabled. As a result the event class shouldn't
escape. If performance metrics highlight an issue, we should revisit.
regards,
Sean.
On 27/06/2018 20:57, Erik Gahlin wrote:
> On 2018-06-27 21:14, Seán Coffey wrote:
>>
>>
>> On 27/06/2018 19:57, Xuelei Fan wrote:
>>> Hi Sean,
>>>
>>> I may reply in several replies.
>>>
>>> PKIXMasterCertPathValidator.java
>>>
>>> + CertChainEvent cce = new CertChainEvent();
>>> + if(cce.isEnabled() || EventHelper.loggingSecurity()) {
>>> + String c = reversedCertList.stream()
>>> + .map(x -> x.getSerialNumber().toString(16))
>>> +.collect(Collectors.joining(", "));
>>> + EventHelper.commitCertChainEvent(cce, c);
>>> + }
>>>
>>> No matter the event or the JFR mechanism is enabled or not, the above
>>> code will create a new instance. Is the return value of
>>> cce.isEnabled() dynamically changed or static?
>> This is a requirement from the JFR framework. All their event.isEnabled
>> calls are instance methods and follow a similar pattern. The JFR team
>> assure me that the JIT can optimize away such calls.
>
> The JIT will most likely not be able to optimize if the event class
> escapes.
>
> From a JFR perspective, this would be the preferred layout:
>
> EventA eventA= new EventA();
> eventA.value = this.value;
> eventA.commit();
>
> and
