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

[Claes Redestad]

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/

/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.redes...@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.redes...@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.mcma...@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.mcma...@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-2...@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