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

List:       openjdk-serviceability-dev
Subject:    Re: RFR (S): JDK-8250630 JdwpListenTest.java fails on Alpine Linux
From:       Dmitry Samersoff <dms () samersoff ! net>
Date:       2020-08-29 8:08:44
Message-ID: ac49e14d-6cf3-4d26-f8b0-1385e9eacc21 () samersoff ! net
[Download RAW message or body]

Hello Alex,

Thank you. Nits fixed in-place.

-Dmitry

On 28.08.2020 23:10, Alex Menkov wrote:
> Hi Dmitry,
> 
> Looks good to me.
> 2 minor nits (no new webrev required):
> - copyright year
> - indentation in lines 763 and 766
> 
> --alex
> 
> On 08/27/2020 08:03, Dmitry Samersoff wrote:
>> Hello Everybody,
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8250630/webrev.02/
>>
>> Webrev is updated, all comments accepted and addressed.
>>
>> Except:
>>  > The error code from the inet_pton is not checked.
>>
>> inet_pton performs conversion of the constant value in our case and 
>> the only possible reason for it to fail is that the system doesn't 
>> support IPv6 at all.
>>
>> -Dmitry
>>
>>
>> On 18.08.2020 3:20, serguei.spitsyn@oracle.com wrote:
>>> Hi Dmitry,
>>>
>>> I agree with Alex, it is better to rename compareIPv6Addr to 
>>> isEqualIPv6Addr.
>>>
>>> 705 static int compareIPv6Addr(struct addrinfo *ai, struct in6_addr 
>>> in6Addr)
>>> 706 {
>>> 707
>>> 708 if (ai->ai_addr->sa_family == AF_INET6) {
>>> 709 const struct sockaddr_in6 sa = *((struct sockaddr_in6*) 
>>> ai->ai_addr);
>>> 710 return (memcmp(&sa.sin6_addr, &in6Addr, sizeof(in6Addr)) == 0);
>>> 711 }
>>> 712
>>> 713 return 0;
>>> 714 }
>>>
>>> I think, the lines 707 and 712 are not needed.
>>>
>>> 725 struct in6_addr mappedAny; ... 761 inet_pton(AF_INET6, 
>>> "::ffff:0.0.0.0", &mappedAny);
>>>
>>> The error code from the inet_pton is not checked.
>>> Also, it can be useful to pre-initialize the mappedAny.
>>>
>>> 737 // Try to find bind address of preferred address familty first A 
>>> dot at the end of comment is missed.
>>>
>>> 745 if (listenAddr == NULL) {
>>> 746 // No address of preferred addres family found, grab the first one
>>> 747 listenAddr = &(addrInfo[0]);
>>>   748     }
>>>
>>> The indent has to be 4, not 3.
>>> The () brackets are not actually needed but I do not object if you 
>>> keep them.
>>> A dot at the end of comment is missed.
>>>
>>> 755 // Binding to IN6ADDR_ANY allow us to serve both IPv4 and IPv6 
>>> connections,
>>> 756 // but binding to mapped INADDR_ANY (::ffff:0.0.0.0) allow us to 
>>> serve IPv4
>>> 757 // connections only. So make sure, that IN6ADDR_ANY is preferred 
>>> over
>>> 758 // mapped INADDR_ANY if preferredAddressFamily is AF_INET6 or not 
>>> set
>>>
>>> I'd suggest to replace "allow us" => "allows" in two places:
>>>    "IN6ADDR_ANY allow us" and "(::ffff:0.0.0.0) allow us".
>>> Also, it'd better to replace: "So make sure,that" => "Make sure that".
>>> A dot at the end of comment is missed.
>>>
>>> I don't know the network protocols well enough to comment on
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 8/17/20 00:21, Dmitry Samersoff wrote:
>>>> Hello Everybody,
>>>>
>>>> Please review the fix:
>>>>
>>>> https://cr.openjdk.java.net/~dsamersoff/JDK-8250630/webrev.01/
>>>>
>>>> Binding to IN6ADDR_ANY allow us to serve both IPv4 and IPv6 
>>>> connections, but binding to mapped INADDR_ANY (::ffff:0.0.0.0) allow 
>>>> us to serve IPv4 connections only.
>>>>
>>>> So make sure, that IN6ADDR_ANY is preferred over mapped INADDR_ANY 
>>>> if preferredAddressFamily is not AF_INET
>>>>
>>>>
>>>> -Dmitry\S
>>>>
>>>
>>

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