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

Mon, 06 May 2019 15:38:52 -0700

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.spit...@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


Reply via email to