Re: RFR 8029995: accept yes/no for boolean krb5.conf settings
Updated webrev at http://cr.openjdk.java.net/~weijun/8029995/webrev.01 Several differences: 1. Only true/false/yes/no are supported now. 2. Some words in javax/security/auth/kerberos/package-info.java 3. getBoolean() renamed to getBooleanObject() because it's quite easy to forget the return value is a Boolean (instead of boolean) and could be null. Thanks Max On Jan 29, 2014, at 5:46, Sean Mullan sean.mul...@oracle.com wrote: On 01/28/2014 03:53 AM, Wang Weijun wrote: Please review the fix at http://cr.openjdk.java.net/~weijun/8029995/webrev.00/ The supported boolean values in this fix cover what MIT krb5 does and we also added 'f'. The old getBooleanValue() method returns true for “true” and false otherwise but the new method returns null if the value is not supported. I’ve carefully changed how the method is called to ensure maximum compatibility, but there is still one left: We support DNS lookup for realm name by default, which means we do it if dns_lookup_realm is not set to false, or when unset, if dns_fallback is not set to false. Before this change, when dns_lookup_realm is set to “unknown”, it means false so DNS lookup is not performed. After this change, it’s equivalent to dns_lookup_realm unset, and dns_fallback is used. I think the current behavior is better than the old one. I agree, but since it is a behavior change, it should be specified in the CCC and release notes. Fix looks fine otherwise. --Sean
RFR 8029995: accept yes/no for boolean krb5.conf settings
Please review the fix at http://cr.openjdk.java.net/~weijun/8029995/webrev.00/ The supported boolean values in this fix cover what MIT krb5 does and we also added 'f'. The old getBooleanValue() method returns true for “true” and false otherwise but the new method returns null if the value is not supported. I’ve carefully changed how the method is called to ensure maximum compatibility, but there is still one left: We support DNS lookup for realm name by default, which means we do it if dns_lookup_realm is not set to false, or when unset, if dns_fallback is not set to false. Before this change, when dns_lookup_realm is set to “unknown”, it means false so DNS lookup is not performed. After this change, it’s equivalent to dns_lookup_realm unset, and dns_fallback is used. I think the current behavior is better than the old one. Thanks Max
Re: RFR 8029995: accept yes/no for boolean krb5.conf settings
On 01/28/2014 03:53 AM, Wang Weijun wrote: Please review the fix at http://cr.openjdk.java.net/~weijun/8029995/webrev.00/ The supported boolean values in this fix cover what MIT krb5 does and we also added 'f'. The old getBooleanValue() method returns true for “true” and false otherwise but the new method returns null if the value is not supported. I’ve carefully changed how the method is called to ensure maximum compatibility, but there is still one left: We support DNS lookup for realm name by default, which means we do it if dns_lookup_realm is not set to false, or when unset, if dns_fallback is not set to false. Before this change, when dns_lookup_realm is set to “unknown”, it means false so DNS lookup is not performed. After this change, it’s equivalent to dns_lookup_realm unset, and dns_fallback is used. I think the current behavior is better than the old one. I agree, but since it is a behavior change, it should be specified in the CCC and release notes. Fix looks fine otherwise. --Sean
Re: RFR 8029995: accept yes/no for boolean krb5.conf settings
In fact, I had thought about throwing an exception when the value is not a supported one, but decided not to do that for compatibility reasons. The old Config::getBooleanValue() has never thrown an exception. We have other methods doing similarly (say, getIntValue()r returns Integer.MIN_VALUE is there is a NumberFormatException). --Max On Jan 29, 2014, at 5:46, Sean Mullan sean.mul...@oracle.com wrote: On 01/28/2014 03:53 AM, Wang Weijun wrote: Please review the fix at http://cr.openjdk.java.net/~weijun/8029995/webrev.00/ The supported boolean values in this fix cover what MIT krb5 does and we also added 'f'. The old getBooleanValue() method returns true for “true” and false otherwise but the new method returns null if the value is not supported. I’ve carefully changed how the method is called to ensure maximum compatibility, but there is still one left: We support DNS lookup for realm name by default, which means we do it if dns_lookup_realm is not set to false, or when unset, if dns_fallback is not set to false. Before this change, when dns_lookup_realm is set to “unknown”, it means false so DNS lookup is not performed. After this change, it’s equivalent to dns_lookup_realm unset, and dns_fallback is used. I think the current behavior is better than the old one. I agree, but since it is a behavior change, it should be specified in the CCC and release notes. Fix looks fine otherwise. --Sean
