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

[Weijun Wang]

Ping again.

The webrev contains codes by myself so I need another reviewer.

Thanks
Max


On 11/09/2012 08:38 AM, Weijun Wang wrote:
> 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
>>>>
>>
>>
>>