[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: [ding] Re: jmx-dev [pong] Re: [ping] Re: RFR 8146015: JMXInterfaceBindingTest is failing intermi
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2016-01-13 18:51:07
Message-ID: 56969C9B.1080104 () oracle ! com
[Download RAW message or body]

On 1/13/16 03:16, Jaroslav Bachorik wrote:
> On 12.1.2016 12:55, serguei.spitsyn@oracle.com wrote:
>> On 1/12/16 03:49, Jaroslav Bachorik wrote:
>>> On 12.1.2016 11:47, serguei.spitsyn@oracle.com wrote:
>>>> On 1/7/16 08:40, Daniel Fuchs wrote:
>>>>> Hi,
>>>>>
>>>>> This looks OK to me.
>>>>> I'm not sure I understand the full impact of the changes
>>>>> in getAddressesForLocalHost() though - so hopefully someone
>>>>> else will jump in to review that part...
>>>>
>>>> Hi Jaroslav,
>>>>
>>>> I do not understand the full impact of the getAddressesForLocalHost()
>>>> change either
>>>> so that, please, do not count me as a reviewer.
>>>
>>> Looks like
>>>
>>>>
>>>> However, I have a question on the fragment:
>>>>
>>>> + private static boolean isLocalhost(InetAddress i) {
>>>> + if (!i.isLoopbackAddress()) {
>>>> + return i.getHostName().toLowerCase().equals("localhost");
>>>> + }
>>>> + return false;
>>>>       }
>>>>
>>>>
>>>> Should true be returned instead of false if the i.isLoopbackAddress()
>>>> returns true?
>>>> Do we normally treat the loopbackAddress case as a localhost variant?
>>>
>>> Ok, maybe the method name is missing a bit - the idea is to get all
>>> 'true' localhosts - a localhost defined on a real non-loopback adapter
>>> (as it doesn't make sense to bind JMX remote connector to a loopback
>>> address).
>>
>> Got it.
>> Thank you for the explanation.
>>
>>
>>>
>>> I just couldn't come up with more describing name not being
>>> excessively long :( I'm open to any suggestions.
>>
>> The name looks Ok to me.
>> I'd suggest to add a short comment with your explanation above.
>
> Thanks. I've added the explanation and also disambiguated the method 
> name a bit.
>
> http://cr.openjdk.java.net/~jbachorik/8146015/webrev.01
>
> It looks like no one else is going to jump in and verify the 
> getAddressesForLocalHost() change (basically, excluding the loopback 
> localhost addresses as they are not suitable for binding the remote 
> JMX connector to) :/ Maybe Severing could comment, as the orignal author?
>
> I would like to integrate this change ASAP since the test is failing 
> 100% in nightly runs.

Ok. I looked  to the getAddressesForLocalHost() part and think this fix 
is fine.
Thumbs up from me.

Thanks,
Serguei

>
> -JB-
>
>>
>>
>> Thanks,
>> Serguei
>>
>>
>>>
>>> -JB-
>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>>
>>>>> best regards,
>>>>>
>>>>> -- daniel
>>>>>
>>>>> On 07/01/16 16:02, Jaroslav Bachorik wrote:
>>>>>> On 5.1.2016 15:30, Jaroslav Bachorik wrote:
>>>>>>> On 4.1.2016 10:05, Jaroslav Bachorik wrote:
>>>>>>>> Gentle reminder ...
>>>>>>>>
>>>>>>>> On 23.12.2015 11:26, Jaroslav Bachorik wrote:
>>>>>>>>> Please, review the following test change
>>>>>>>>>
>>>>>>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8146015
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8146015/webrev.00
>>>>>>>>>
>>>>>>>>> The test fails for IPv6 addresses since the RMI expects an IPv6
>>>>>>>>> address
>>>>>>>>> to be properly wrapped in '[]'. In addition to that the logic for
>>>>>>>>> selecting IP addresses to bind is flawed - it does not check 
>>>>>>>>> for IP
>>>>>>>>> addresses of multiple adapters but for multiple IP addresses
>>>>>>>>> assigned to
>>>>>>>>> 'localhost'. In combination with IPv4 & IPv6 this will cause the
>>>>>>>>> test to
>>>>>>>>> attempt binding to IPv4 and IPv6 address of the same adapter
>>>>>>>>> simultaneously and the test will fail.
>>>>>>>>>
>>>>>>>>> The fix adds the requested wrapping for IPv6 addresses and 
>>>>>>>>> adjusts
>>>>>>>>> the
>>>>>>>>> IP selection logic to iterate over distinct adapters first and
>>>>>>>>> prevent
>>>>>>>>> IPv4 and IPv6 address of the same adapter being treated as two
>>>>>>>>> addresses
>>>>>>>>> (for the purposes of the test).
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> -JB-
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]