Thanks for the review, jack. Please find the new patch that: 1. checks for tps null value in TPSTokenPolicy constructor 2. for the methods with Hashtables passing around, I added comments to explain what they are, and renamed the methods to be more clear on what they do. However, I need time to think about creating classes for them as I'm not very certain we want to do that at the moment. I will add the discussion to the next patch, if it's okay with you,. 3. I moved the check with revokeCert config to the top of the method. Finally, as I stated, there are improvements to come for the recovery decision method. thanks, Christina On 08/18/2014 07:16 PM, John Magne wrote: > 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(a)redhat.com&gt; > To: pki-devel(a)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(a)redhat.com > https://www.redhat.com/mailman/listinfo/pki-devel _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel