Thanks for the review Valerie. Comments below.
On 26 Jun 2013, at 00:23, Valerie (Yu-Ching) Peng wrote: > > <Secmod.java> > - The private static native boolean nssInit(...) method should be removed? It > seems obsolete by nssInitWithOptions(...) and I didn't see it being used > anywhere. Same goes for the corresponding JNI method impl, i.e. > Java_sun_security_pkcs11_Secmod_nssInit, in the j2secmod.c file. I considered zapping nssInit() but I wanted to ensure I didn't break anything in the JDK 7 Update release. I can remove it from the JDK 8 version. > > <Config.java> > - Particular reason to use different variable name, i.e. > "nssUseOptimizeSpace" and property/option name "nssOptimizeSpace"? It seems > inconsistent as most config variable name matches the property/option name. > Can't we just use "nssOptimizeSpace"across board? The previous name of the SunPKCS11 configuration flag was 'nssUseOptimizeSpace'. I'll update all the internal references. > > <j2secmod.c> > - line 127, empty string is used instead of the passed configDir. I am not > sure if this correct. Why not just pass the configDir to this call? To be consistent with the NSS native code: it passes an empty string. > > Lastly, is the "NSS_Initialize(...)" method always available for the > supported NSS library versions, i.e. 3.7+? Is this a newer method meant to > replace "NSS_Init(...)"? It's available in addition to the NSS_Init* functions (not a replacement). > Thanks, > Valerie > > On 06/19/13 10:38, Vincent Ryan wrote: >> Thanks for the review. I've simplified the name of the NSS flag, updated the >> bug report and filed a doc bug, >> as you suggest. >> >> >> >> On 19 Jun 2013, at 18:21, Sean Mullan wrote: >> >>> Looks good, just a couple of comments: >>> >>> 1. I think the name "nssOptimizeSpace" is clearer. The "Use" part seems a >>> bit odd in the property name. >>> >>> 2. Add the appropriate noreg label to the bug. >>> >>> 3. File a followup doc bug to document the attribute in the PKCS11 guide. >>> >>> --Sean >>> >>> On 06/19/2013 08:49 AM, Vincent Ryan wrote: >>>> I've made some corrections to the native method that initializes NSS. >>>> The new webrev is at: >>>> >>>> http://cr.openjdk.java.net/~vinnie/7165807/webrev.01 >>>> >>>> >>>> >>>> On 14 Jun 2013, at 18:38, Vincent Ryan wrote: >>>> >>>>> Please review the following fix: >>>>> >>>>> http://cr.openjdk.java.net/~vinnie/7165807/webrev.00/ >>>>> http://bugs.sun.com/view_bug.do?bug_id=7165807 >>>>> >>>>> NSS may be initialized to favour performance or to favour memory >>>>> footprint. >>>>> This fix introduces a new configuration flag to allow Java applications >>>>> to choose. >>>>> By default, NSS will be initialized for performance. >>>>> >>>>> Thanks. >>>>> >
