Comments: -This method: public TPSTokenPolicy (TPSSubsystem tps) { + this.tps = tps; + // init from config first + String policySetString = getDefaultPolicySetString(); + parsePolicySetString(policySetString); Maybe just check for a null tps in the constructor and throw an exception. Then we don't have to worry about tps being invalid again. -This method: public Hashtable<String, TokenRecord> tdbFindTokenRecords(String uid) + throws Exception { OK, there are a bunch of places where we are passing back and passing around these hashbables of tokens or certs or whatever. First we have no idea what is in the the hash table unless inspecting the code closely. Second. It looks like this method is takin an Iterator returned by the DB code and then making this hash table. I propose that we create classes for this such as "TPSTokenRecordList" and then provide convenient methods to return the entries to the caller. We could even just use the Iterator returned before. Then when we pass this stuff to other functions , we pass the convenient class. -This method: + private TPSStatus generateCertsAfterRenewalRecoveryPolicy(EnrolledCertsInfo certsInfo, SecureChannel channel, AppletInfo aInfo) + throws TPSException { Just restating what you said earlier in that this thing is way to complex and confusing to digest. It needs simplification. - This method: protected void revokeCertificates(String cuid) throws TPSException { Here we wait until we are like 70% of the way through the processing to see if we are even configured to revoke certificates. I see that in the beginning we try to cull out already revoked certs, but this could be done in another method or just wait for the next time we actually need to revoke anything. ----- Original Message ----- From: "Christina Fu" <cfu@redhat.com> To: pki-devel@redhat.com Sent: Monday, August 18, 2014 6:56:17 PM Subject: [Pki-devel] [PATCH] pki-cfu-0027-ticket-882-tokendb-policy-handling-revocation-and-re.patch This is continuation of token policy work from ticket #882 This patch provides 1. the 2nd part (and not last) of the token policy work - determining policies for Enrollment, Re-enrollment, Renewal, Recovery, and Revocation (note: Recovery decision needs more refinement in later patch. The generateCertsAfterRenewalRecoveryPolicy method is currently just a close translation from the original TPS and is extremely complex. It deserves some close inspection and improvement at later time, in next round) 2. revocation for certificates due to token operations (note: administrator initiated will be handled in later patch) thanks, Christina _______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel