Looks like this should work, but still find it complicated.

1. Do we need to care about thread safety when managing legacyStrings?

2. Does implReplaceAll() need to set legacyChanged = true?

3. How about using prngAlgorithms.iterator().next() below?

  1416     return prngAlgorithms.toArray(new String[0])[0];

--Max


> On Jun 6, 2020, at 11:54 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
> 
> Thanks for reviewing and sharing the feedbacks on webrev.00.
> 
> In order to support all existing calls for legacy registration for default 
> secure random, I have to revert some of the JDK-7092821 changes and 
> re-introduce the 'legacyStrings' LinkedHashMap. Updated the regression test 
> with removal test for provider using legacy registrations as well. Although 
> removal is supported, this is still not bullet proof as things may not work 
> as expected if a provider registered their impl in both ways, i.e. legacy 
> String pair and Service, and then remove/replace some entries later. Please 
> comment if you really need this scenario to be supported. Although not 
> explicitly documented, I think the intention is to use one or the other, 
> never both.
> 
> Webrev update: http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/
> 
> Thanks,
> Valerie
> On 6/5/2020 11:00 AM, Valerie Peng wrote:
>> Right, I try to keep the impl simple as I am not aware of any property 
>> removal usage.
>> 
>> Oh-well, if we have to cater to the removal scenario, then we'd have to add 
>> a List and keep track of ALL secure random algos instead of only the FIRST 
>> one. Alright, I guess it costs for covering all aspect. But one extra 
>> benefit of this is that it should be easy to handle the future JDK property 
>> such as "jdk.securerandom.disabledAlgorithms" if it were to be added.
>> 
>> Valerie
>> 
>> On 6/5/2020 7:54 AM, Weijun Wang wrote:
>>> I don't know who in this world would want to do that, but Prasad's concern 
>>> is technically possible. I tried 'p.remove("SecureRandom.a")' in the new 
>>> test, and new SecureRandom() fails with 
>>> "java.security.NoSuchAlgorithmException: a SecureRandom not available".
>>> 
>>> And people can also remove one entry and add it back in order to move it to 
>>> the end. One can even add new implementations this way.
>>> 
>>> Unfortunately there is no ConcurrentLinkedHashMap.
>>> 
>>> --Max
>>> 
>>>> On Jun 5, 2020, at 1:44 PM, Prasadrao Koppula 
>>>> <prasadarao.kopp...@oracle.com> wrote:
>>>> 
>>>> Hi,
>>>> 
>>>> Looks good to me, one question
>>>> 
>>>> If first registered SecureRandom algo gets removed, 
>>>> getDefaultSecureRandomAlgorithm return stale data, a refresh required in 
>>>> remove?
>>>> 
>>>> Thanks,
>>>> Prasad.K
>>>> 
>>>>> -----Original Message-----
>>>>> From: Valerie Peng
>>>>> Sent: Friday, June 5, 2020 2:52 AM
>>>>> To: security-dev@openjdk.java.net
>>>>> Subject: Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo
>>>>> based on registration ordering
>>>>> 
>>>>> Hi, Sean,
>>>>> 
>>>>> Thanks for the review and feedback. Webrev updated:
>>>>> http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/
>>>>> 
>>>>> getTypeAndAlgorithm(...) was not static due to an instance variable used 
>>>>> by
>>>>> debugging output. I have removed it and made both method static.
>>>>> 
>>>>> I will wait for others' review comments also.
>>>>> 
>>>>> Thanks,
>>>>> Valerie
>>>>> On 6/4/2020 2:01 PM, Sean Mullan wrote:
>>>>>> On 6/4/20 3:34 PM, Valerie Peng wrote:
>>>>>>> Hi,
>>>>>>> 
>>>>>>> Could someone help reviewing this fix? This change keep tracks of the
>>>>>>> first registered SecureRandom algorithm and returns it upon the
>>>>>>> request of SecureRandom class.
>>>>>> This looks good to me. I would recommend that Max or someone else
>>>>>> review it as well.
>>>>>> 
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8246613
>>>>>>> 
>>>>>>> Webrev: http://cr.openjdk.java.net/~valeriep/8246613/webrev.00/
>>>>>> A couple of minor comments, feel free to fix or ignore.
>>>>>> 
>>>>>> * SecureRandom.java
>>>>>> 
>>>>>> 879             // For SUN provider, we use
>>>>>> SunEntries.DEFF_SECURE_RANDOM_ALGO
>>>>>> 
>>>>>> Might as well fix the typo while you are in there: s/DEFF/DEF/
>>>>>> 
>>>>>> * Provider.java
>>>>>> 
>>>>>> 1156     private String parseSecureRandomPut(String name, String
>>>>>> value) {
>>>>>> 
>>>>>> Could be static if you also make getTypeAndAlgorithm static, I think.
>>>>>> 
>>>>>> --Sean

Reply via email to