RFR 7191662: JCE providers should be located via ServiceLoader,

Fri May 29 01:05:40 UTC 2015

Please find comments in line.

On 5/27/2015 4:40 PM, Mandy Chung wrote:
>> On May 27, 2015, at 4:17 PM, Valerie Peng<valerie.peng at oracle.com>  wrote:
>>
>> Hi, Mandy,
>>
>> I have second thought on using the provider name for the java.security file...
>> There are a few things that I want to discuss/brainstorm internally to make sure that there is no "surprise" later.
>> Thus, I had not made the switch in this webrev. It doesn't affect the overall functionality.
>>
> What is the issue you concern about with ServiceLoader to load providers when it matches its provider name rather than the classname?  SPI allows you to move away from using the classname in the configuration and separate implementation from interfaces.
There are several things...or questions that I'd like to clarify before 
making the switch.
Not all of them are related to this switch though, but still, I don't 
want to complicate things further by adding new changes into this mix.

First, provider name seems "looser" than provider class name. With just 
a name, what happens when there is a name collision?
Is there an ordering on how ServiceLoader loads/looks up services? If 
yes/no, does it affect provider validation?
Second, it's a bit awkward in the SunPKCS11 provider case. For SunPKCS11 
provider, its name is set in the configuration file which is passed to 
the provider object upon the configure call. When ServiceLoader loads 
the SunPKCS11 provider object, you can't use the provider name to look 
for a match as the provider itself doesn't have that info yet. Sure, we 
can always do special handling for SunPKCS11 to make this work. But this 
special handling isn't needed for the class name setting.
Lastly, this overlaps with Tony's JEP and his current proposal fits 
better with class name setting (this may just be my personal opinion). 
So, I feel it's better to clarify all these questions before making the 
switch.

I did mention about this (not using provider name in java.security) when 
sending out my webrev to security alias but didn't not notify Alan and 
you separately. Sorry~
Valerie
>
>
>> The legacyLoad() is for finding providers the old-fashioned way, e.g. through reflection, instead of ServiceLoader.
>> The legacyLoad can only work using class name.
>>
> For the first push, it’s okay to leave them loaded by the legacy mechanism via reflection until you sort out the provider name issue before you make them true service providers.   But I still think ProviderLoader.load should not tie with the classname unless there is a strong reason to.
>
>
>> Running test/java/lang/SecurityManager/CheckSecurityProvider.java with a security manager should cover more than without.
> Ok with me.
>
>> I don't see much value in adding provider name verification? Verifying class name seems enough in making sure that the providers are present as expected.
>>
> If you are using the provider name to load service providers, this adds additional verification.  I’m surprised that your webrev was changed as I didn’t see any mail about your concern that leads to the change.
>
>> As for test/tools/launcher/MiscTests.java, it's for verifying a bug in launcher, so I have stick with the internal API as I am not sure if switching to public API may be too great of a change as the call path changed significantly…
> This test verifies 6856415 that needs to use sun.* API and ensures that the right Exception is thrown.  Ok - it’s okay to leave it then.
>
> Mandy
>
>> Thanks,
>> Valerie
>>
>> On 5/26/2015 3:27 PM, Mandy Chung wrote:
>>> sun/security/jca/ProviderConfig.java
>>>
>>> 287         /**
>>> 288          * Loads the provider with the specified class name.
>>> 289          *
>>> 290          * @param name the name of the provider class
>>> 291          * @return the Provider, or null if it cannot be found or loaded
>>> 292          * @throws ProviderException all other exceptions are ignored
>>> 293          */
>>> 294         public Provider load(String classname) {
>>>                             :
>>>
>>> 305                     if (p.getClass().getName().equals(classname)) {
>>> 306                         return p;
>>> 307                     }
>>>
>>> Is this load method supposed to take the provider name instead of the classname?
>>> line 305 should check against p.getName() instead?  The legacyLoad method is
>>> for the fallback to the class name.
>>>
>>> java.security now uses classname.  Did you decide to use security provider name instead?
>>> Even so, the legacyLoader method should be able to find them and the load method
>>> looks to me should check the provider name instead.
>>>
>>> test/java/lang/SecurityManager/CheckSecurityProvider.java
>>>    you add this test to run with security manager.  I wonder if you want
>>>    to run with and without security manager through @run othervm.
>>>
>>>    Now each security provider has a name.  Should this test verify the provider
>>>    name as well?
>>>
>>> test/tools/launcher/MiscTests.java
>>>    - does this test need to call sun.security.pkcs11 internal API? Can this be
>>>      changed to call public API?
>>>
>>> I didn't review other files.
>>> Mandy
>>>
>>> On 05/26/2015 01:57 PM, Sean Mullan wrote:
>>>> This all looks fine to me (except for the Makefile stuff which I'll leave to others).
>>>>
>>>> --Sean
>>>>
>>>> On 05/21/2015 12:21 AM, Valerie Peng 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
>>>>>
>>>>> Thanks,
>>>>> Valerie
>>>>>