RFR [9] 8040837: Avoid provoking NFEs when initializing InetAddrCachePolicy

[Chris Hegarty]

On 21 Apr 2014, at 20:11, Claes Redestad <claes.redestad at oracle.com> wrote:

> Hi again,
> 
> please review the latest revision, which fixes a small discrepancy where Security.getProperty was used instead of System.getProperty (implicitly called by Integer.getInteger in the original code):
> 
> http://cr.openjdk.java.net/~mduigou/JDK-8040837/1/webrev/

These changes look good to me.

-Chris.

> 
> /Claes
> 
> On 2014-04-18 22:52, Mike Duigou wrote:
>> Updated webrev from Claes:
>> 
>> http://cr.openjdk.java.net/~mduigou/JDK-8040837/1/webrev/
>> 
>> On Apr 18 2014, at 11:02 , claes.redestad <claes.redestad at oracle.com> wrote:
>> 
>>> Looks like you're right, unfortunately. Seems I confused System.getProperty and Security.getProperty, and of course they differ subtly. I'll fix it as soon as I can, but I'm out for a few hours..
>>> 
>>> /Claes
>>> 
>>> 
>>> -------- Originalmeddelande --------
>>> Från: Bernd Eckenfels
>>> Datum:18-04-2014 19:06 (GMT+01:00)
>>> Till: Michael McMahon
>>> Kopia: Mike Duigou , claes.redestad at oracle.com
>>> Rubrik: Re: RFR [9] 8040837: Avoid provoking NFEs when initializing InetAddrCachePolicy
>>> 
>>> Hello,
>>> 
>>> I think the updated revision also uses Security.getProperty for the
>>> fallbacks, I think this is wrong, they are read from the System
>>> properties. (can be read with Integer.getProperty which uses internally
>>> decode).
>>> 
>>> (and sorry for the big discussion on such a small patch :)
>>> 
>>> Gruss
>>> Bernd
>>> 
>>> Am Fri, 18 Apr 2014 17:57:04 +0100 schrieb Michael McMahon
>>> <michael.x.mcmahon at oracle.com>:
>>> 
>>>> Thanks Mike (and adding Claes back in)
>>>> 
>>>> Is there a reason to use both Integer.decode() and Integer.valueOf()?
>>>> 
>>>> Michael
>>>> 
>>>> On 18/04/14 17:44, Mike Duigou wrote:
>>>>> Claes tidied things up to produce a workable patch:
>>>>> 
>>>>>> Here is the updated webrev:
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~mduigou/JDK-8040837/0/webrev/
>>>>>> 
>>>>>> I will push it to jdk9/dev/jdk on Friday before COB for Claes
>>>>>> unless I hear objections.
>>>>>> 
>>>>>> Cheers,
>>>>>> 
>>>>>> Mike
>>>>> On Apr 18 2014, at 09:27 , Michael McMahon
>>>>> <michael.x.mcmahon at oracle.com> wrote:
>>>>> 
>>>>>> I think it would be an improvement to combine these doPrivileged()
>>>>>> blocks as suggested, though your patch needs work Bernd. For
>>>>>> instance, the multi-catch doesn't work. Also the
>>>>>> PrivilegedAction<> type is wrong.
>>>>>> 
>>>>>> If someone wants to update it, then we can use that. Otherwise,
>>>>>> we'll go with the original suggested change.
>>>>>> 
>>>>>> Thanks
>>>>>> Michael
>>>>>> 
>>>>>> On 17/04/14 21:28, Bernd Eckenfels wrote:
>>>>>>> Am Thu, 17 Apr 2014 21:50:23 +0200
>>>>>>> schrieb Bernd Eckenfels <bernd-2014 at eckenfels.net>:
>>>>>>> 
>>>>>>>> Hello,
>>>>>>>> 
>>>>>>>> I would propose to use Integer.valueOf(tmp) instead, but looking
>>>>>>>> at the context I think it is even better to skip this and the
>>>>>>>> following null check with Integer.parseInt().
>>>>>>> This is even shorter and it reduces the privileged actions
>>>>>>> invocations:
>>>>>>> 
>>>>>>>         Integer tmp = java.security.AccessController.doPrivileged
>>>>>>> ( new PrivilegedAction<String>() {
>>>>>>>             public String run() {
>>>>>>>                 try {
>>>>>>>                     String tmpString =
>>>>>>> Security.getProperty(cachePolicyProp); if (tmpString != null)
>>>>>>> return Integer.valueOf(tmpString); } catch (NumberFormatException
>>>>>>> | IllegalArgumentException ignored) { }
>>>>>>> 
>>>>>>>                 try {
>>>>>>>                     return
>>>>>>> Integer.getInteger(cachePolicyPropFallback); } catch
>>>>>>> (NumberFormatException | IllegalArgumentException ignored) { }
>>>>>>> 
>>>>>>>                 return null;
>>>>>>>             }
>>>>>>>         });
>>>>>>> 
>>>>>>>          if (tmp != null) {
>>>>>>>              cachePolicy = tmp.intValue();
>>>>>>>              if (cachePolicy < 0) {
>>>>>>>                  cachePolicy = FOREVER;
>>>>>>>              }
>>>>>>>              propertySet = true;
>>>>>>>          } else {
>>>>>>>              /* No properties defined for positive caching. If
>>>>>>> there is no
>>>>>>>               * security manager then use the default positive
>>>>>>> cache value. */ if (System.getSecurityManager() == null) {
>>>>>>>                  cachePolicy = DEFAULT_POSITIVE;
>>>>>>>              }
>>>>>>>          }
>>>>>>> 
>>>>>>> NB: this will
>