On 21 Apr 2014, at 20:11, Claes Redestad <claes.redes...@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.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 >
