> On May 20, 2015, at 9:21 PM, Valerie Peng <valerie.p...@oracle.com> wrote:
> 
> Sean,
> 
> Could you please review this change? The changes are mostly the same as the 
> prototype in Jake, but I have to make some modification due to the difference 
> in ServiceLoader lookup in OpenJDK (corresponding 
> META-INF/services/java.security.Provider files in each module) and the 
> related makefile change (merge their content into one for the final image 
> build). Then, I adjusted the Provider.configure() method to take a single 
> String argument to be consistent with the "providerarg" option that keytool 
> defined.
> 
> In addition, I also made some misc changes, such as configuring the providers 
> inside ProviderConfig instead of ProviderLoader, add back the doPrivileged 
> block to all the provider constructors. I also have second thought on making 
> the switch to privider name (instead of provider class name) in java.security 
> file, so I reverted the changes on that - that SunPKCS11 provider has its 
> name specified in its configuration file, so when ServiceLoader loads the 
> PKCS11 provider, the configuration file has not been passed to it, so the 
> name is not known at that time. Thus, using the class name for the provider 
> list entry seems to fit the flow better. I also updated the default policy 
> for SunPKCS11 provider given its recent change of using sun.misc.
> 
> Webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/
> CCC: http://ccc.us.oracle.com/7191662 <http://ccc.us.oracle.com/7191662>


A quick comment on the META-INF/services config files and the makefile. Merging 
the service config files is temporary until the module system is moving further 
along.

src/jdk.crypto.ucrypto/solaris/classes/META-INF/services/java.security.Provider.html
and other os-specific service configuration:
   1 #[solaris]com.oracle.security.ucrypto.UcryptoProvider

- why is this commented out?  Does the makefile uncomment it?  It should be 
simple
concatenation with

The makefile doesn’t seem right though.

make/gensrc/Gensrc-java.naming.gmk.html

  96 jdk.crypto.ec: $(GENSRC_PROVIDER)
  98 all: jdk.crypto.ec

java.naming doesn’t seem an appropriate module to be the main module for 
containing all service provider config files.  I initially propose to use 
jdk.crypto.ec as the gensrc module as indicated in line 96,98.

You can rename the file to Gensrc-jdk.crypto.ec and update the content..

GENSRC_PROVIDER := 
$(SUPPORT_OUTPUTDIR)/gensrc/java.naming/META-INF/services/java.security.Provider

GENSRC_PROVIDER is the output file.  line 79-89 is building the target list.   
I think you need another variable to build up the target list but not 
GENSRC_PROVIDER.

You can reference how Gensrc-jdk.jdi.gmk concatenates the service config for 
jdk.jdi and dk.hotspot.agent module.

# Filter com.sun.jdi.connect.Connector
$(SUPPORT_OUTPUTDIR)/gensrc/jdk.jdi/META-INF/services/com.sun.jdi.connect.Connector:
 \
    
$(JDK_TOPDIR)/src/jdk.jdi/share/classes/META-INF/services/com.sun.jdi.connect.Connector
 \
    $(SUPPORT_OUTPUTDIR)/gensrc/jdk.hotspot.agent/_the.sa.services
        $(process-provider)

# Copy the same service file into jdk.hotspot.agent so that they are kept the 
same.
$(JDK_OUTPUTDIR)/modules/jdk.hotspot.agent/META-INF/services/com.sun.jdi.connect.Connector:
 \
    
$(SUPPORT_OUTPUTDIR)/gensrc/jdk.jdi/META-INF/services/com.sun.jdi.connect.Connector
        $(install-file)

Mandy

Reply via email to