PING: Re: RFR: JDK-8184770: JDWP support for IPv6

[serguei.spitsyn at oracle.com]

Hi guys,

We need a couple of partial reviews for this enhancement:

  - From the net-dev to check IPv6-addresses related part.
    It does not need to be a thorough review.
    We need another pair of eyes to check for obvious typos or misses.
    Included Chris H. to mailing list in hope for some assistance.
    (Chris, we need some help to find one of the IPv6 experts.)

  - From the serviceability-dev to check if compatibility
    is not broken and nothing is missed.
    This includes part of code that is not directly involved
    into manipulations with the IPv4/IPv6 addresses.
    The related spec update is tracked by a sub-task:
      https://bugs.openjdk.java.net/browse/JDK-8221707


Related CSR:
https://bugs.openjdk.java.net/browse/JDK-8223104

The latest webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.04/


Thanks,
Serguei


On 5/6/19 3:27 PM, serguei.spitsyn at oracle.com wrote:
> Hi Alex,
>
> It looks great and very solid in general!
>
> Some minor comments are below.
>
> http://cr.openjdk.java.net/~amenkov/IPv6/webrev.04/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html
>
> 263 * Result must be release with dbgsysFreeAddrInfo.
>   A typo: "must be release" => "must be released"
>
>    80 typedef struct {
> 81 struct in6_addr subnet;
> 82 struct in6_addr netmask;
>    83 } AllowedPeerInfo;
>   . . .
>
> 431 parseAllowedPeersInternal(char *buffer) { . . .
> 483 } . . . 524 isPeerAllowed(struct sockaddr_storage *peer) {
> 525 struct in6_addr tmp;
> 526 struct in6_addr *addr6;
> 527 if (peer->ss_family == AF_INET) {
> 528 convertIPv4ToIPv6((struct sockaddr *)peer, &tmp);
> 529 addr6 = &tmp;
> 530 } else {
> 531 addr6 = &(((struct sockaddr_in6 *)peer)->sin6_addr);
> 532 }
> 533
> 534 for (int i = 0; i < _peers_cnt; ++i) {
> 535 if (isAddressInSubnet(addr6, &(_peers[i].subnet), 
> &(_peers[i].netmask))) {
>   536             return 1;
>   537         }
>   538     }
>  The allowed pears are converted into and used/checked in the IPv6 format.
>  Some short comments about it in all three places above would be helpful.
>  I'd consider to do the same in parseAllowedAddr() before this fragment:
> 367 if (addrInfo->ai_family == AF_INET6) {
> 368 memcpy(result, &(((struct sockaddr_in6 
> *)(addrInfo->ai_addr))->sin6_addr), sizeof(*result));
> 369 *isIPv4 = 0;
> 370 } else { // IPv4 address
> 371 struct in6_addr addr6;
> 372 convertIPv4ToIPv6(addrInfo->ai_addr, &addr6);
> 373 memcpy(result, &addr6, sizeof(*result));
> 374 *isIPv4 = 1;
>   375     }
>
>  and in parseAllowedMask() before the line:
> 420 result->s6_addr[i] = (char)(0xFF << (8 - prefixLen));
>
> This fragment needs a comment: 402 if (isIPv4) {
> 403 prefixLen += 96;
>   404     }
>
>
> We have to find out at least one more reviewer for this fix!
>
> Thanks,
> Serguei
>
>
> On 5/3/19 18:14, serguei.spitsyn at oracle.com wrote:
>> Hi Alex,
>>
>> Thank you for creating the CSR!
>> I've added myself as a reviewer and corrected a couple of places.
>> Feel free to change my changes if necessary. :)
>> It would be nice to get one more CSR review if possible, so I've 
>> added the net-dev mailing list.
>>
>> I hope to finish the webrev review soon.
>>
>> Thanks,
>> Serguei
>>
>> On 5/1/19 10:54 AM, Alex Menkov wrote:
>>> Hi all,
>>>
>>> I created CSR for the feature:
>>> https://bugs.openjdk.java.net/browse/JDK-8223104
>>>
>>> The latest webrev:
>>> http://cr.openjdk.java.net/~amenkov/IPv6/webrev.04/
>>>
>>> --alex
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190507/0a67c875/attachment.html>