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

[Dmitry Samersoff]

Weijun,

Config.java:1162
     This code is unclear to me.
     if srvs[i] could be "" this code could insert extra space in
     the middle of kdcs string.

     if srvc[i] couldn't be empty we can return null just
     after line 1160  if srvs.length == 0

KrbServiceLocator.java:285

    Kerberos could be run on non standard port - rare case but
    AFAIK we don't limit it. So it's better to use parseInt here.

dns.sh:
	Why we need Shell script here?

Regards,
-Dmitry



On 2012-11-13 15:05, Weijun Wang wrote:
> 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
>>>>>
>>>
>>>
>>>


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer