Erik,
comments inline..
On 08/11/18 15:12, Erik Gahlin wrote:
Hi Sean,
I think we could still call the event
"jdk.SecurityPropertyModification", but add a @Description that
explains that events are only emitted for the JDK due to security
concerns. If we at a later stage want to include user events, we could
add those and remove the @Description, possibly with a setting where
you can specify scope, or perhaps a new event all together.
sounds fine. I've made those changes.
Perhaps we could find a better name for the field
validationEventNumber? It sounds like we have an event number, which
is not really the case. We have a counter for the validation id.
How about 'validationCounter' ?
I noticed that you use hashCode as an id. I assume this is to simplify
the implementation? That is probably OK, the risk of a collision is
perhaps low, but could you make the field a long, so we in the future
could add something more unique (Flight Recorder uses compressed
integers, so a long uses the same amount of disk space as an int).
Could you also rename the field and the annotation, perhaps
certificationId could work, so we don't leak how the id was implemented.
Yes - originally, I was using the certificate serial number but that may
not always be unique (esp. for test generated certs). I've changed the
variable name to 'certificateId' and made it a long. Renamed the
annotation also.
I could not find the file:
test/lib/jdk/test/lib/security/TestJdkSecurityPropertyModification.java
whoops - forgot to add it to mercurial tracking. there now :
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v8/webrev/
regards,
Sean.
