Looks good, some comments: 1. public void tdbAddCertificatesForCUID(String cuid, ArrayList<TPSCertRecord> certs, ExternalRegAttrs erAttrs) + throws TPSException { + String method = "TPSTokendb.tdbAddCertificatesForCUID (with erAttrs): "; + CMS.debug(method + "begins"); + if (cuid == null || certs== null || certs.isEmpty() || erAttrs == null) { + CMS.debug(method + "params cuid, certs and erAttrs cannot be null or empty"); + } Here we do a bunch of sanity checking but fail to throw the exception. 2. There are a few TPSException(s) thrown but no code is specified, taking whatever the default is. 3. The little inner enum class should probably be positioned at the bottom on the class. ----- Original Message ----- From: "Christina Fu" <cfu(a)redhat.com&gt; To: pki-devel(a)redhat.com Sent: Friday, May 22, 2015 5:34:30 PM Subject: [Pki-devel] [PATCH] pki-cfu-0070-Ticket-1309-Recovering-of-a-revoked-cert-erroneously.patch This patch addresses issues reported in: https://fedorahosted.org/pki/ticket/1309Recovering of a revoked cert erroneously reflects "active" in the token db cert entry A new config param has been introduced to allow/disallow of recovering a revoked certificate for externalReg: externalReg.allowRecoverInvalidCert.enable=true by default it is true Please review. thanks, Christina _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

Looks good. ACK ----- Original Message ----- From: "Christina Fu" <cfu(a)redhat.com&gt; To: pki-devel(a)redhat.com Sent: Friday, May 22, 2015 7:21:28 PM Subject: Re: [Pki-devel] [PATCH] pki-cfu-0070-Ticket-1309-Recovering-of-a-revoked-cert-erroneously.patch Hi John, Thanks for the review. Please see this updated patch (pki-cfu-0072). thanks, Christina On 05/22/2015 07:03 PM, John Magne wrote: > Looks good, some comments: > > 1. public void tdbAddCertificatesForCUID(String cuid, ArrayList<TPSCertRecord> certs, ExternalRegAttrs erAttrs) > + throws TPSException { > + String method = "TPSTokendb.tdbAddCertificatesForCUID (with erAttrs): "; > + CMS.debug(method + "begins"); > + if (cuid == null || certs== null || certs.isEmpty() || erAttrs == null) { > + CMS.debug(method + "params cuid, certs and erAttrs cannot be null or empty"); > + } > > Here we do a bunch of sanity checking but fail to throw the exception. > > > 2. There are a few TPSException(s) thrown but no code is specified, taking whatever the default is. > > 3. The little inner enum class should probably be positioned at the bottom on the class. > > > > ----- Original Message ----- > From: "Christina Fu" <cfu(a)redhat.com&gt; > To: pki-devel(a)redhat.com > Sent: Friday, May 22, 2015 5:34:30 PM > Subject: [Pki-devel] [PATCH] pki-cfu-0070-Ticket-1309-Recovering-of-a-revoked-cert-erroneously.patch > > This patch addresses issues reported in: > https://fedorahosted.org/pki/ticket/1309Recovering of a revoked cert > erroneously reflects "active" in the token db cert entry > > A new config param has been introduced to allow/disallow of recovering a > revoked certificate for externalReg: > externalReg.allowRecoverInvalidCert.enable=true > by default it is true > > Please review. > 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