8002344 code review request: (was Re: [PATCH] for bug 2376501: Krb5LoginModule config class does not return proper KDC list from DNS)

[Weijun Wang]

Hi Severin

I've created an OpenJDK bug and created a new webrev:

    http://cr.openjdk.java.net/~weijun/8002344/webrev.00/

The Config.java change is identical to yours, and I added a small tweak 
in KrbServiceLocator, and a quite ugly regression test.

Anyone can review all the changes?

After the code review, I'll push the change to tl/jdk. I don't see an 
OpenJDK user id for you at http://db.openjdk.java.net/people, so I add 
your name in

   Contributed-by: Severin Gehwolf <sgehwolf at redhat.com>

Thanks
Max


On 11/08/2012 11:46 PM, Severin Gehwolf wrote:
> Hi Max,
>
> Thanks for the review!
>
> On Wed, 2012-11-07 at 08:52 +0800, Weijun Wang wrote:
>> The fix looks fine. There is one place it might get enhanced:
>>
>>           if (value.charAt(j) == ':') {
>>               kdcs = (value.substring(0, j)).trim();
>>           }
>>
>> So this changes a.com:88 to a.com. If the port is really 88, it's OK.
>> Otherwise, info gets lost. Maybe we can keep the whole string.
>
> I've removed the entire loop which strips the port from the returned
> record. Updated webrev is here:
>
> http://jerboaa.fedorapeople.org/bugs/openjdk/2376501/webrev.1/
>
>> BTW, are you OK with contributing the fix into OpenJDK main repo?
>
> Yes, of course :) Just let me know what's to be done to get it pushed.
>
> Cheers,
> Severin
>
>> On 11/06/2012 11:08 PM, Severin Gehwolf wrote:
>>> Hi,
>>>
>>> In Config.java, line 1234 in method getKDCFromDNS(String realm) there is
>>> a loop which discards earlier values of KDCs returned rather than
>>> concatenating them. This results in a behaviour where only one KDC in a
>>> seemingly random fashion is returned. In fact, the KDC returned depends
>>> on the order which KrbServiceLocator.getKerberosService(realm, "_udp")
>>> returns the servers. The correct behaviour should be to return a String
>>> containing ALL KDCs available via DNS (separated by spaces).
>>>
>>> The webrev is here:
>>> http://jerboaa.fedorapeople.org/bugs/openjdk/2376501/webrev/
>>>
>>> Comments and suggestions very welcome!
>>>
>>> Thanks,
>>> Severin
>>>
>
>
>