Request for review: ECC support for pkisilent (CA, subCA, DRM, TKS, TPS, OCSP)
This is a request for code review for the following feature bug: *Bug 810967* <https://bugzilla.redhat.com/show_bug.cgi?id=810967> -[RFE] ECC support for pkisilent the code changes can be found here: https://bugzilla.redhat.com/attachment.cgi?id=699587&action=diff&context=patch&collapsed=&headers=1&format=raw The 5 new templates and one readme instruction file can be found the the bug attachment. thanks! Christina
Comments: In general, the code changes look fine. Just a couple of nitpicks .. 1. The variable save_p12 should be a boolean rather than a string. 2. In ComCrypto.java, you define setTokenName(). What about getTokenName()? 3. In ComCrypto.java,on line 448, you cast token to (PK11Token) and on 450, there is no cast. Why the discrepancy? Is the cast needed? 4. The exception handling in ComCrypto.java for lines 448/450 is pretty wonky. It seems like token could return null. Could we improve the exception handling in this case? Ade On Tue, 2The 013-02-19 at 11:26 -0800, Christina Fu wrote:
This is a request for code review for the following feature bug: Bug 810967 - [RFE] ECC support for pkisilent
the code changes can be found here: https://bugzilla.redhat.com/attachment.cgi?id=699587&action=diff&context=patch&collapsed=&headers=1&format=raw
The 5 new templates and one readme instruction file can be found the the bug attachment.
thanks! Christina
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
Ade, Thank you for the review. Per our discussion. The agreed changes can now be found at: https://bugzilla.redhat.com/attachment.cgi?id=699762&action=diff&context=patch&collapsed=&headers=1&format=raw In addition, I have added changes to the admin cert profile to accept ECC: https://bugzilla.redhat.com/attachment.cgi?id=699763&action=diff&context=patch&collapsed=&headers=1&format=raw Also, I have now put the ECC setup instruction at this location: http://pki.fedoraproject.org/wiki/ECC_Setup_Instructions So the silentEC_readme.txt now only contains link to it: https://bugzilla.redhat.com/attachment.cgi?id=699764 thanks! Christina On 02/19/2013 12:04 PM, Ade Lee wrote:
Comments:
In general, the code changes look fine. Just a couple of nitpicks ..
1. The variable save_p12 should be a boolean rather than a string.
2. In ComCrypto.java, you define setTokenName(). What about getTokenName()?
3. In ComCrypto.java,on line 448, you cast token to (PK11Token) and on 450, there is no cast. Why the discrepancy? Is the cast needed?
4. The exception handling in ComCrypto.java for lines 448/450 is pretty wonky. It seems like token could return null. Could we improve the exception handling in this case?
Ade
On Tue, 2The 013-02-19 at 11:26 -0800, Christina Fu wrote:
This is a request for code review for the following feature bug: Bug 810967 - [RFE] ECC support for pkisilent
the code changes can be found here: https://bugzilla.redhat.com/attachment.cgi?id=699587&action=diff&context=patch&collapsed=&headers=1&format=raw
The 5 new templates and one readme instruction file can be found the the bug attachment.
thanks! Christina
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
Christina, The changes you have put in ComCrypto.java look great. I did notice, however, that you do a similar token assignment around line 710, but still have the unnecessary cast, no check for null and wonky exception handling. Please fix this instance too. Once that is done, we can ACK the patch. Ade On Tue, 2013-02-19 at 18:04 -0800, Christina Fu wrote:
Ade,
Thank you for the review.
Per our discussion. The agreed changes can now be found at: https://bugzilla.redhat.com/attachment.cgi?id=699762&action=diff&context=patch&collapsed=&headers=1&format=raw
In addition, I have added changes to the admin cert profile to accept ECC: https://bugzilla.redhat.com/attachment.cgi?id=699763&action=diff&context=patch&collapsed=&headers=1&format=raw
Also, I have now put the ECC setup instruction at this location: http://pki.fedoraproject.org/wiki/ECC_Setup_Instructions So the silentEC_readme.txt now only contains link to it: https://bugzilla.redhat.com/attachment.cgi?id=699764
thanks! Christina
On 02/19/2013 12:04 PM, Ade Lee wrote:
Comments:
In general, the code changes look fine. Just a couple of nitpicks ..
1. The variable save_p12 should be a boolean rather than a string.
2. In ComCrypto.java, you define setTokenName(). What about getTokenName()?
3. In ComCrypto.java,on line 448, you cast token to (PK11Token) and on 450, there is no cast. Why the discrepancy? Is the cast needed?
4. The exception handling in ComCrypto.java for lines 448/450 is pretty wonky. It seems like token could return null. Could we improve the exception handling in this case?
Ade
On Tue, 2The 013-02-19 at 11:26 -0800, Christina Fu wrote:
This is a request for code review for the following feature bug: Bug 810967 - [RFE] ECC support for pkisilent
the code changes can be found here: https://bugzilla.redhat.com/attachment.cgi?id=699587&action=diff&context=patch&collapsed=&headers=1&format=raw
The 5 new templates and one readme instruction file can be found the the bug attachment.
thanks! Christina
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
Hi Ade, Please find the following changes as discussed: https://bugzilla.redhat.com/attachment.cgi?id=700248&action=diff&context=patch&collapsed=&headers=1&format=raw https://bugzilla.redhat.com/attachment.cgi?id=700250&action=diff&context=patch&collapsed=&headers=1&format=raw thanks! Christina On 02/19/2013 07:28 PM, Ade Lee wrote:
Christina,
The changes you have put in ComCrypto.java look great. I did notice, however, that you do a similar token assignment around line 710, but still have the unnecessary cast, no check for null and wonky exception handling. Please fix this instance too.
Once that is done, we can ACK the patch.
Ade
On Tue, 2013-02-19 at 18:04 -0800, Christina Fu wrote:
Ade,
Thank you for the review.
Per our discussion. The agreed changes can now be found at: https://bugzilla.redhat.com/attachment.cgi?id=699762&action=diff&context=patch&collapsed=&headers=1&format=raw
In addition, I have added changes to the admin cert profile to accept ECC: https://bugzilla.redhat.com/attachment.cgi?id=699763&action=diff&context=patch&collapsed=&headers=1&format=raw
Also, I have now put the ECC setup instruction at this location: http://pki.fedoraproject.org/wiki/ECC_Setup_Instructions So the silentEC_readme.txt now only contains link to it: https://bugzilla.redhat.com/attachment.cgi?id=699764
thanks! Christina
On 02/19/2013 12:04 PM, Ade Lee wrote:
Comments:
In general, the code changes look fine. Just a couple of nitpicks ..
1. The variable save_p12 should be a boolean rather than a string.
2. In ComCrypto.java, you define setTokenName(). What about getTokenName()?
3. In ComCrypto.java,on line 448, you cast token to (PK11Token) and on 450, there is no cast. Why the discrepancy? Is the cast needed?
4. The exception handling in ComCrypto.java for lines 448/450 is pretty wonky. It seems like token could return null. Could we improve the exception handling in this case?
Ade
On Tue, 2The 013-02-19 at 11:26 -0800, Christina Fu wrote:
This is a request for code review for the following feature bug: Bug 810967 - [RFE] ECC support for pkisilent
the code changes can be found here: https://bugzilla.redhat.com/attachment.cgi?id=699587&action=diff&context=patch&collapsed=&headers=1&format=raw
The 5 new templates and one readme instruction file can be found the the bug attachment.
thanks! Christina
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
participants (2)
-
Ade Lee -
Christina Fu