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

2019-05-15 Thread Chris Hegarty 

Alex,

On 13/05/2019 22:06, Alex Menkov wrote:

Hi Chris, Serguei,

Updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.05/


I'm ok with this version.


CSR (approved):
https://bugs.openjdk.java.net/browse/JDK-8223104

Changes (vs. webrev.04):

- setsockopt(IPV6_V6ONLY) was moved from Win-specific code to shared 
setOptionsCommon function (in socketTransport.c)
   the value by default is "true" on Windows and is "false" on Unix, but 
it can be overridden, so lets set it explicitly;

- a comment why the result of setsockopt(IPV6_V6ONLY) is ignored is added;
- added some comments as per Serguei request.

About scopes support - I thought that the functionality is not important 
for debugger, but I can implement it (I'd prefer to do it by separate RFE).


Ok.

-Chris.


--alex

On 05/11/2019 03:39, Chris Hegarty wrote:




On 7 May 2019, at 19:40, serguei.spit...@oracle.com wrote:

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/



src/jdk.jdwp.agent/windows/native/libdt_socket/socket_md.c

    223 if (domain == AF_INET6) {
    224 int off = 0;
    225 // make the socket a dual mode socket
    226 setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, (char 
*), sizeof(off));

    227 }
    228   }

   This code is fine, but maybe you want to expand the comment to
   mention that the setsockopt may fail if IPv4 is not supported. And
   that’s OK.

   I cannot find a similar setting of IPV6_V6ONLY for the unix
   equivalent. Why not, or where can it be found?

src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c

   There is a lot of native code here, I skimmed over it, seems
   reasonable. There is an obvious lack of any reference to IPv6
   scopes. Can the address given to bind be an IPv6 link-local?
   If so, then the scope will need to be parsed / set appropriately.
   ( seems that the new test skips all scoped addresses? )

-Chris.

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

2019-05-13 Thread serguei.spit...@oracle.com 

Hi Alex,

Thank you for the update!
It looks good to me.

Thanks,
Serguei


On 5/13/19 14:06, Alex Menkov wrote:

Hi Chris, Serguei,

Updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.05/
CSR (approved):
https://bugs.openjdk.java.net/browse/JDK-8223104

Changes (vs. webrev.04):

- setsockopt(IPV6_V6ONLY) was moved from Win-specific code to shared 
setOptionsCommon function (in socketTransport.c)
  the value by default is "true" on Windows and is "false" on Unix, 
but it can be overridden, so lets set it explicitly;
- a comment why the result of setsockopt(IPV6_V6ONLY) is ignored is 
added;

- added some comments as per Serguei request.

About scopes support - I thought that the functionality is not 
important for debugger, but I can implement it (I'd prefer to do it by 
separate RFE).


--alex

On 05/11/2019 03:39, Chris Hegarty wrote:




On 7 May 2019, at 19:40, serguei.spit...@oracle.com wrote:

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/



src/jdk.jdwp.agent/windows/native/libdt_socket/socket_md.c

    223 if (domain == AF_INET6) {
    224 int off = 0;
    225 // make the socket a dual mode socket
    226 setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, (char 
*), sizeof(off));

    227 }
    228   }

   This code is fine, but maybe you want to expand the comment to
   mention that the setsockopt may fail if IPv4 is not supported. And
   that’s OK.

   I cannot find a similar setting of IPV6_V6ONLY for the unix
   equivalent. Why not, or where can it be found?

src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c

   There is a lot of native code here, I skimmed over it, seems
   reasonable. There is an obvious lack of any reference to IPv6
   scopes. Can the address given to bind be an IPv6 link-local?
   If so, then the scope will need to be parsed / set appropriately.
   ( seems that the new test skips all scoped addresses? )

-Chris.

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

2019-05-13 Thread Alex Menkov 

Hi Chris, Serguei,

Updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.05/
CSR (approved):
https://bugs.openjdk.java.net/browse/JDK-8223104

Changes (vs. webrev.04):

- setsockopt(IPV6_V6ONLY) was moved from Win-specific code to shared 
setOptionsCommon function (in socketTransport.c)
  the value by default is "true" on Windows and is "false" on Unix, but 
it can be overridden, so lets set it explicitly;

- a comment why the result of setsockopt(IPV6_V6ONLY) is ignored is added;
- added some comments as per Serguei request.

About scopes support - I thought that the functionality is not important 
for debugger, but I can implement it (I'd prefer to do it by separate RFE).


--alex

On 05/11/2019 03:39, Chris Hegarty wrote:




On 7 May 2019, at 19:40, serguei.spit...@oracle.com wrote:

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/



src/jdk.jdwp.agent/windows/native/libdt_socket/socket_md.c

223 if (domain == AF_INET6) {
224 int off = 0;
225 // make the socket a dual mode socket
226 setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, (char *), 
sizeof(off));
227 }
228   }

   This code is fine, but maybe you want to expand the comment to
   mention that the setsockopt may fail if IPv4 is not supported. And
   that’s OK.

   I cannot find a similar setting of IPV6_V6ONLY for the unix
   equivalent. Why not, or where can it be found?

src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c

   There is a lot of native code here, I skimmed over it, seems
   reasonable. There is an obvious lack of any reference to IPv6
   scopes. Can the address given to bind be an IPv6 link-local?
   If so, then the scope will need to be parsed / set appropriately.
   ( seems that the new test skips all scoped addresses? )

-Chris.

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

2019-05-11 Thread Chris Hegarty 



> On 7 May 2019, at 19:40, serguei.spit...@oracle.com wrote:
> 
> 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/


src/jdk.jdwp.agent/windows/native/libdt_socket/socket_md.c

   223 if (domain == AF_INET6) {
   224 int off = 0;
   225 // make the socket a dual mode socket
   226 setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, (char *), 
sizeof(off));
   227 }
   228   }

  This code is fine, but maybe you want to expand the comment to
  mention that the setsockopt may fail if IPv4 is not supported. And
  that’s OK.

  I cannot find a similar setting of IPV6_V6ONLY for the unix 
  equivalent. Why not, or where can it be found?

src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c

  There is a lot of native code here, I skimmed over it, seems
  reasonable. There is an obvious lack of any reference to IPv6
  scopes. Can the address given to bind be an IPv6 link-local?
  If so, then the scope will need to be parsed / set appropriately.
  ( seems that the new test skips all scoped addresses? )

-Chris.

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

2019-05-07 Thread Arthur Eubanks 
Even though I showed our internal patches in this area earlier, I haven't
really looked into exactly what this patch does (since it was written by
someone else).
I can confirm that it fixes jshell in an IPv6 only environment though.

*From: *
*Date: *Tue, May 7, 2019 at 11:42 AM
*To: *Alex Menkov, , net-dev, Chris
Hegarty

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.spit...@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, ); 529 
> addr6 =  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, 
> ); 373 memcpy(result, , 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
>
>
>
>
>

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

2019-05-07 Thread serguei . spitsyn 

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.spit...@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, );
529 addr6 = 
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, );
373 memcpy(result, , 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

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

2019-05-06 Thread serguei.spit...@oracle.com 

  
  
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, );
 529 addr6 = 
 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, );
 373 memcpy(result, , 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

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

2019-05-03 Thread serguei . spitsyn 

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

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

2019-05-01 Thread Alex Menkov 

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

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

2019-04-23 Thread Alex Menkov 

Hi Serguei,

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

Fixed everything except #7 (for consistency - in the file if function 
arguments spread on several lines, opening curly bracket is placed to 
separate line to separate function declaration from the function body).


--alex

On 04/12/2019 18:52, serguei.spit...@oracle.com wrote:

Hi Alex,

Thank you for the update!

One more round of minor formatting change requests for better 
readability. :)


#1:

http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/src/jdk.jdi/share/classes/com/sun/tools/jdi/SocketTransportService.java.udiff.html

+    port = Integer.decode(hostPort.substring(splitIndex+1));
. . .
+    } else if (hostPort.charAt(0) == ‘[’ && 
hostPort.charAt(splitIndex-1) == ‘]’) {


  Need spaces around ‘+’ and ‘-’ signs.

#2:

http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/src/jdk.jdwp.agent/windows/native/libdt_socket/socket_md.c.udiff.html

+    //make the socket a dual mode socket

  missed space at the start of comment


Now, comments for:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html

#3:

276 /* check for host:port or port */
277 colon = strrchr(address, ‘:’);
278 port = (colon == NULL ? address : colon + 1);

279 /* ensure the port is valid (getaddrinfo allows port to be empty) */
280 if (getPortNumber(port) < 0) {

#4:

298 hints.ai_family = allowOnlyIPv4 ? AF_INET : AF_INET6;
299 hints.ai_flags |= AI_PASSIVE | (allowOnlyIPv4 ? 0 : 
AI_V4MAPPED | AI_ALL);

300 
301 } else {

#5: Replace “fills” with “fills in” in:

341  * Parses address (IPv4 or IPv6), fills result by parsed address.
383  * Parses prefix length from buffer (integer value), fills result 
with corresponding net mask.
485  * Parses ‘allow’ argument (fills list of allowed peers (global 
_peers variable)).


#6:

410 // generate mask for prefix length
411 memset(result, 0, sizeof(*result));

412 // prefixLen <= 128, so we won’t go over result’s size
413 for (int i = 0; prefixLen > 0; i++, prefixLen -= :sunglasses: {

#7:

623 socketTransport_startListening(jdwpTransportEnv* env, const char* 
address,

624    char** actualAddress)
625 {
. . . .
1173 static int readBooleanSysProp(int *result, int trueValue, int 
falseValue,
1174 JNIEnv* jniEnv, jclass sysClass, jmethodID getPropMethod, const 
char *propName)

1175 {

  Move ‘{’ to the end of 624 and 1174. (edited)

#8:

1176 jstring value;
1177 jstring name = (*jniEnv)->NewStringUTF(jniEnv, propName);

1178 if (name == NULL) {
. . . .
1259 } while (0);

1260 if (jniEnv != NULL && (*jniEnv)->ExceptionCheck(jniEnv)) {


Thanks!
Serguei


On 4/12/19 4:58 PM, Alex Menkov wrote:

updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/

changes (vs webrev.02) are non-functional (added/edited comments, code 
reformatting, renaming convertIpv4ToIpv6 function to 
convertIPv4ToIPv6, renaming env variable to jniEnv (env is already 
used in one of the functions)).


About pass/preferredAddressFamily conditions - there is no "logical 
xor" in C/C++. Also I think that the current condition is clearer.


--alex

On 04/11/2019 17:18, serguei.spit...@oracle.com wrote:

Hi Alex,

Great debugging feature!
While I'm still reading all the details, could you, please, fix some 
minor format issues?


http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdi/share/classes/com/sun/tools/jdi/SocketTransportService.java.udiff.html 



+ * If host is a literal IPv6 address, it may be in 
square brackets. Extra space before "square".



http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html 



  I'd suggest to unify comments before functions:
   - start comment with a capital latter and ended with a dot
   - use comment format like this:
    /*
 */

  Examples of comments that need this change:

262 /* result must be release with dbgsysFreeAddrInfo */ => /* * 
Result must be release with dbgsysFreeAddrInfo.  */


325 // input is sockaddr just because all clients have it =>
/* * Input is sockaddr just because all clients have it. */

1129 /* reads boolean system value,
1130 * sets *result to trueValue if the ptoperty is "true",
1131 * to falseValue if the property if "false",
1132 * doesn't change *result if the property is not set or failed to 
read.
1133 */ => /* * Read boolean system value andset result to: * - 
trueValue if the property is "true"
* - falseValue if the property is "false" *   * Return JNI_OK if 
result is set, return JNI_ERR  otherwise.

*/

  . . .

293 * use IPv6 socket (to accept IPv6 and mapped Ipv4),...
342 * (with AF_INET6 Ipv4 addresses are not parsed even with 
AI_V4MAPPED and AI_ALL flags) ...345 hints.ai_family = AF_UNSPEC; // 
IPv6 or mapped Ipv4 ... 360 } else { // Ipv4 address Replace Ipv4

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

2019-04-12 Thread serguei . spitsyn 

Hi Alex,

Thank you for the update!

One more round of minor formatting change requests for better 
readability. :)


#1:

http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/src/jdk.jdi/share/classes/com/sun/tools/jdi/SocketTransportService.java.udiff.html

+    port = Integer.decode(hostPort.substring(splitIndex+1));
. . .
+    } else if (hostPort.charAt(0) == ‘[’ && 
hostPort.charAt(splitIndex-1) == ‘]’) {


 Need spaces around ‘+’ and ‘-’ signs.

#2:

http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/src/jdk.jdwp.agent/windows/native/libdt_socket/socket_md.c.udiff.html

+    //make the socket a dual mode socket

 missed space at the start of comment


Now, comments for:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html

#3:

276 /* check for host:port or port */
277 colon = strrchr(address, ‘:’);
278 port = (colon == NULL ? address : colon + 1);

279 /* ensure the port is valid (getaddrinfo allows port to be empty) */
280 if (getPortNumber(port) < 0) {

#4:

298 hints.ai_family = allowOnlyIPv4 ? AF_INET : AF_INET6;
299 hints.ai_flags |= AI_PASSIVE | (allowOnlyIPv4 ? 0 : 
AI_V4MAPPED | AI_ALL);

300 
301 } else {

#5: Replace “fills” with “fills in” in:

341  * Parses address (IPv4 or IPv6), fills result by parsed address.
383  * Parses prefix length from buffer (integer value), fills result 
with corresponding net mask.
485  * Parses ‘allow’ argument (fills list of allowed peers (global 
_peers variable)).


#6:

410 // generate mask for prefix length
411 memset(result, 0, sizeof(*result));

412 // prefixLen <= 128, so we won’t go over result’s size
413 for (int i = 0; prefixLen > 0; i++, prefixLen -= :sunglasses: {

#7:

623 socketTransport_startListening(jdwpTransportEnv* env, const char* 
address,

624    char** actualAddress)
625 {
. . . .
1173 static int readBooleanSysProp(int *result, int trueValue, int 
falseValue,
1174 JNIEnv* jniEnv, jclass sysClass, jmethodID getPropMethod, const 
char *propName)

1175 {

 Move ‘{’ to the end of 624 and 1174. (edited)

#8:

1176 jstring value;
1177 jstring name = (*jniEnv)->NewStringUTF(jniEnv, propName);

1178 if (name == NULL) {
. . . .
1259 } while (0);

1260 if (jniEnv != NULL && (*jniEnv)->ExceptionCheck(jniEnv)) {


Thanks!
Serguei


On 4/12/19 4:58 PM, Alex Menkov wrote:

updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/

changes (vs webrev.02) are non-functional (added/edited comments, code 
reformatting, renaming convertIpv4ToIpv6 function to 
convertIPv4ToIPv6, renaming env variable to jniEnv (env is already 
used in one of the functions)).


About pass/preferredAddressFamily conditions - there is no "logical 
xor" in C/C++. Also I think that the current condition is clearer.


--alex

On 04/11/2019 17:18, serguei.spit...@oracle.com wrote:

Hi Alex,

Great debugging feature!
While I'm still reading all the details, could you, please, fix some 
minor format issues?


http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdi/share/classes/com/sun/tools/jdi/SocketTransportService.java.udiff.html 



+ * If host is a literal IPv6 address, it may be in 
square brackets. Extra space before "square".



http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html 



  I'd suggest to unify comments before functions:
   - start comment with a capital latter and ended with a dot
   - use comment format like this:
    /*
 */

  Examples of comments that need this change:

262 /* result must be release with dbgsysFreeAddrInfo */ => /* * 
Result must be release with dbgsysFreeAddrInfo.  */


325 // input is sockaddr just because all clients have it =>
/* * Input is sockaddr just because all clients have it. */

1129 /* reads boolean system value,
1130 * sets *result to trueValue if the ptoperty is "true",
1131 * to falseValue if the property if "false",
1132 * doesn't change *result if the property is not set or failed to 
read.
1133 */ => /* * Read boolean system value andset result to: * - 
trueValue if the property is "true"
* - falseValue if the property is "false" *   * Return JNI_OK if 
result is set, return JNI_ERR  otherwise.

*/

  . . .

293 * use IPv6 socket (to accept IPv6 and mapped Ipv4),...
342 * (with AF_INET6 Ipv4 addresses are not parsed even with 
AI_V4MAPPED and AI_ALL flags) ...345 hints.ai_family = AF_UNSPEC; // 
IPv6 or mapped Ipv4 ... 360 } else { // Ipv4 address Replace Ipv4 
with IPv4 for unification with IPv6

For unification replace: convertIpv4ToIpv6 => convertIPv4ToIPv6

297 hints.ai_flags |= AI_PASSIVE
298 | (allowOnlyIPv4 ? 0 : AI_V4MAPPED | AI_ALL); Better to have just 
one line



1135 JNIEnv* env, . . . 1165 JNIEnv* jniEnv = NULL;

A suggestion is to use the same name for JNIEnv*:

   1135 JNIEnv* jni, . . .
   1165

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

2019-04-12 Thread Alex Menkov 

updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/

changes (vs webrev.02) are non-functional (added/edited comments, code 
reformatting, renaming convertIpv4ToIpv6 function to convertIPv4ToIPv6, 
renaming env variable to jniEnv (env is already used in one of the 
functions)).


About pass/preferredAddressFamily conditions - there is no "logical xor" 
in C/C++. Also I think that the current condition is clearer.


--alex

On 04/11/2019 17:18, serguei.spit...@oracle.com wrote:

Hi Alex,

Great debugging feature!
While I'm still reading all the details, could you, please, fix some 
minor format issues?


http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdi/share/classes/com/sun/tools/jdi/SocketTransportService.java.udiff.html

+ * If host is a literal IPv6 address, it may be in square 
brackets. Extra space before "square".



http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html

  I'd suggest to unify comments before functions:
   - start comment with a capital latter and ended with a dot
   - use comment format like this:
    /*
     */

  Examples of comments that need this change:

262 /* result must be release with dbgsysFreeAddrInfo */ => /* * Result 
must be release with dbgsysFreeAddrInfo.  */


325 // input is sockaddr just because all clients have it =>
/* * Input is sockaddr just because all clients have it. */

1129 /* reads boolean system value,
1130 * sets *result to trueValue if the ptoperty is "true",
1131 * to falseValue if the property if "false",
1132 * doesn't change *result if the property is not set or failed to read.
1133 */ => /* * Read boolean system value andset result to: * - 
trueValue if the property is "true"

* - falseValue if the property is "false" *   * Return JNI_OK if result is set, 
return JNI_ERR  otherwise.
*/

  . . .

293 * use IPv6 socket (to accept IPv6 and mapped Ipv4),...
342 * (with AF_INET6 Ipv4 addresses are not parsed even with AI_V4MAPPED 
and AI_ALL flags) ...345 hints.ai_family = AF_UNSPEC; // IPv6 or mapped 
Ipv4 ... 360 } else { // Ipv4 address Replace Ipv4 with IPv4 for 
unification with IPv6

For unification replace: convertIpv4ToIpv6 => convertIPv4ToIPv6

297 hints.ai_flags |= AI_PASSIVE
298 | (allowOnlyIPv4 ? 0 : AI_V4MAPPED | AI_ALL); Better to have just 
one line



1135 JNIEnv* env, . . . 1165 JNIEnv* jniEnv = NULL;

A suggestion is to use the same name for JNIEnv*:

   1135 JNIEnv* jni, . . .
   1165 JNIEnv* jni = NULL;


   Reformat:

608 if ((pass == 0 && ai->ai_family == preferredAddressFamily) 609 || 
(pass == 1 && ai->ai_family != preferredAddressFamily)) and


828 if ((pass == 0 && ai->ai_family == preferredAddressFamily) 829 || 
(pass == 1 && ai->ai_family != preferredAddressFamily)) => if ((pass == 
0 && ai->ai_family == preferredAddressFamily) || (pass == 1 && 
ai->ai_family != preferredAddressFamily))


   Even better, replace it with logical XOR:

 if ((pass == 0 ^^ ai->ai_family == preferredAddressFamily)


http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/ListeningConnector/startListening/startlis001.java.frames.html

102 /* Check that listening address returned by 
ListeningConnector.startListening()

103 * matches the address which was set via connector's arguments.
104 * Empty host address causes listening for local connections only 
(loopback interface)

105 * */ Dot is missed at the end. Replace "* */" with "*/".


http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/test/jdk/com/sun/jdi/JdwpAllowTest.java.frames.html

162 // generate allow address by changing random bit in the local address
163 // and calculate 2 masks (prefix length) - one is matches original 
local address

164 // and another doesn't. Replace with /* style of comment.


249 positiveTest("PositiveMaskTest(" + test.localAddress + ")", 
test.allowAddress + "/" + test.prefixLengthGood);
250 positiveTest("NegativeMaskTest(" + test.localAddress + ")", 
test.allowAddress + "/" + test.prefixLengthBad);


   A suggestion to move second argument to additional line:

positiveTest("PositiveMaskTest(" + test.localAddress + ")", 
test.allowAddress + "/" + test.prefixLengthGood);
positiveTest("NegativeMaskTest(" + test.localAddress + ")", 
test.allowAddress + "/" + test.prefixLengthBad);


Thanks,
Serguei


On 4/2/19 4:14 PM, Alex Menkov wrote:

Updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/

- added support for addresses enclosed in square brackets;
- updated SocketTransportService.java to handle empty hostname the 
same way as JDWP agent (listen/attach to loopback address);

Has to update nsk/jdi/ListeningConnector/startListening/startlis001.java
(all other com/sun/jdi, nsk/jdi, nsk/jdwp, nsk/jdb test are passed).

--alex

On 04/01/2019 11:21, Alex Menkov wrote:

Hi Chris,

On 04/01/2019 04:50, Chris Hegarty wrote:

Alex,

On 29/03/2019 22:07, Alex Menkov wrote:

(added net-dev as suggested by Alan)

Net

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

2019-04-11 Thread serguei . spitsyn 

Hi Alex,

Great debugging feature!
While I'm still reading all the details, could you, please, fix some 
minor format issues?


http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdi/share/classes/com/sun/tools/jdi/SocketTransportService.java.udiff.html

+ * If host is a literal IPv6 address, it may be in square 
brackets. Extra space before "square".



http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html

 I'd suggest to unify comments before functions:
  - start comment with a capital latter and ended with a dot
  - use comment format like this:
   /*
    */

 Examples of comments that need this change:

262 /* result must be release with dbgsysFreeAddrInfo */ => /* * Result 
must be release with dbgsysFreeAddrInfo.  */


325 // input is sockaddr just because all clients have it =>
/* * Input is sockaddr just because all clients have it. */

1129 /* reads boolean system value,
1130 * sets *result to trueValue if the ptoperty is "true",
1131 * to falseValue if the property if "false",
1132 * doesn't change *result if the property is not set or failed to read.
1133 */ => /* * Read boolean system value andset result to: * - 
trueValue if the property is "true"

* - falseValue if the property is "false" *   * Return JNI_OK if result is set, 
return JNI_ERR  otherwise.
*/

 . . .

293 * use IPv6 socket (to accept IPv6 and mapped Ipv4),...
342 * (with AF_INET6 Ipv4 addresses are not parsed even with AI_V4MAPPED 
and AI_ALL flags) ...345 hints.ai_family = AF_UNSPEC; // IPv6 or mapped 
Ipv4 ... 360 } else { // Ipv4 address Replace Ipv4 with IPv4 for 
unification with IPv6

For unification replace: convertIpv4ToIpv6 => convertIPv4ToIPv6

297 hints.ai_flags |= AI_PASSIVE
298 | (allowOnlyIPv4 ? 0 : AI_V4MAPPED | AI_ALL); Better to have just 
one line



1135 JNIEnv* env, . . . 1165 JNIEnv* jniEnv = NULL;

A suggestion is to use the same name for JNIEnv*:

  1135 JNIEnv* jni, . . .
  1165 JNIEnv* jni = NULL;


  Reformat:

608 if ((pass == 0 && ai->ai_family == preferredAddressFamily) 609 || 
(pass == 1 && ai->ai_family != preferredAddressFamily)) and


828 if ((pass == 0 && ai->ai_family == preferredAddressFamily) 829 || 
(pass == 1 && ai->ai_family != preferredAddressFamily)) => if ((pass == 
0 && ai->ai_family == preferredAddressFamily) || (pass == 1 && 
ai->ai_family != preferredAddressFamily))


  Even better, replace it with logical XOR:

if ((pass == 0 ^^ ai->ai_family == preferredAddressFamily)


http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/ListeningConnector/startListening/startlis001.java.frames.html

102 /* Check that listening address returned by 
ListeningConnector.startListening()

103 * matches the address which was set via connector's arguments.
104 * Empty host address causes listening for local connections only 
(loopback interface)

105 * */ Dot is missed at the end. Replace "* */" with "*/".


http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/test/jdk/com/sun/jdi/JdwpAllowTest.java.frames.html

162 // generate allow address by changing random bit in the local address
163 // and calculate 2 masks (prefix length) - one is matches original 
local address

164 // and another doesn't. Replace with /* style of comment.


249 positiveTest("PositiveMaskTest(" + test.localAddress + ")", 
test.allowAddress + "/" + test.prefixLengthGood);
250 positiveTest("NegativeMaskTest(" + test.localAddress + ")", 
test.allowAddress + "/" + test.prefixLengthBad);


  A suggestion to move second argument to additional line:

positiveTest("PositiveMaskTest(" + test.localAddress + ")", 
test.allowAddress + "/" + test.prefixLengthGood);
positiveTest("NegativeMaskTest(" + test.localAddress + ")", 
test.allowAddress + "/" + test.prefixLengthBad);


Thanks,
Serguei


On 4/2/19 4:14 PM, Alex Menkov wrote:

Updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/

- added support for addresses enclosed in square brackets;
- updated SocketTransportService.java to handle empty hostname the 
same way as JDWP agent (listen/attach to loopback address);

Has to update nsk/jdi/ListeningConnector/startListening/startlis001.java
(all other com/sun/jdi, nsk/jdi, nsk/jdwp, nsk/jdb test are passed).

--alex

On 04/01/2019 11:21, Alex Menkov wrote:

Hi Chris,

On 04/01/2019 04:50, Chris Hegarty wrote:

Alex,

On 29/03/2019 22:07, Alex Menkov wrote:

(added net-dev as suggested by Alan)

Net gurus, please assist in reviewing socket-related code.

New webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.01/


Specifically on SocketTransportService.java. What Arthur has
proposed is better ( changing to lastIndexOf alone is not
sufficient ). Or is your assumption that the IPv6 literal is
not enclosed in square brackets?


I didn't know about enclosing IPv6 in square brackets, but looks like 
that's standard way to alleviate conflict between IPv6 address and 
colon as port separator.
Will

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

2019-04-02 Thread Alex Menkov 

Updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/

- added support for addresses enclosed in square brackets;
- updated SocketTransportService.java to handle empty hostname the same 
way as JDWP agent (listen/attach to loopback address);

Has to update nsk/jdi/ListeningConnector/startListening/startlis001.java
(all other com/sun/jdi, nsk/jdi, nsk/jdwp, nsk/jdb test are passed).

--alex

On 04/01/2019 11:21, Alex Menkov wrote:

Hi Chris,

On 04/01/2019 04:50, Chris Hegarty wrote:

Alex,

On 29/03/2019 22:07, Alex Menkov wrote:

(added net-dev as suggested by Alan)

Net gurus, please assist in reviewing socket-related code.

New webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.01/


Specifically on SocketTransportService.java. What Arthur has
proposed is better ( changing to lastIndexOf alone is not
sufficient ). Or is your assumption that the IPv6 literal is
not enclosed in square brackets?


I didn't know about enclosing IPv6 in square brackets, but looks like 
that's standard way to alleviate conflict between IPv6 address and colon 
as port separator.
Will update the fix to handle them in both JDI connectors 
(SocketTransportService.java) and debugger agent (socketTransport.c)




If keeping Arthur's `static class HostPort` please make the
fields final.

 >> Would it make sense to support the preference properties?
 >>    java.net.preferIPv4Stack
 >>    java.net.preferIPv6Addresses

I'm not sure about this, especially given the property name
prefixes. I need to think a little more on it.


In the initial version of the fix I didn't check the properties.
The rationale here is backward compatibility - is address is empty, old 
debuggers tries to connect to IPv4 address only.
But handling this properties we will better handle clients with 
properties set (as jdb or any other debugger which uses JDI connectors 
are affected by the properties).


BTW fix provided by Arthur implements listening on localhost differently 
- it creates several sockets and binds them to both IPv4 and IPv6 
addresses. But the problem here (and that's the reason I decide to not 
implement it this way) - how to handle the case when we successfully 
bind on one address (for example IPv4), but fail to bind on other (for 
example the port is busy for IPv6 stack). Arthur's version just fail in 
the case (i.e. the whole Java process terminates) and I don't think this 
is good behavior.





There is quite a bit of new native code, is it possible to
rewrite any of this in Java, e.g. reading of the system
properties ( if that is to remain )?


socketTransport.c is a debugger agent which is completely native.

--alex



-Chris.

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

2019-04-01 Thread Alex Menkov 

Hi Daniel,

As far as I see you are right.
Attach to address with empty hostname will try to connect to "external" 
address instead of loopback.


--alex

On 04/01/2019 02:51, Daniel Fuchs wrote:

Hi Arthur,

Not directly related to Alex's original question but...

On 30/03/2019 00:03, Arthur Eubanks wrote:
We have some ipv6 patches as well in this area as well (as well as 
other patches to support ipv6 only environments) that we're trying to 
upstream. I don't understand the code myself, but it might be useful 
to take a look:

http://cr.openjdk.java.net/~aeubanks/jdwpipv6/webrev.00/index.html


SocketTransportService.java:

On my machine at least, InetAddress.getByName("localhost") and
InetAddress.getLocalHost() are quite different:


InetAddress.getByName("localhost") will return the loopback (127.0.0.1)
InetAddress.getLocalHost() returns the local hostname (dhcp-XXX-XXX...)

If I'm not mistaken your changes in HostPort would transform something
that previously returned the loopback (no host specified) with something
that returns the local host name ("*" specified).


So I'm not sure these changes are quite right.
Maybe Alex will be able to confirm.

best regards,

-- daniel

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

2019-04-01 Thread Alex Menkov 



On 04/01/2019 05:02, Chris Hegarty wrote:



On 01/04/2019 12:50, Chris Hegarty wrote:

...
Specifically on SocketTransportService.java. What Arthur has
proposed is better ( changing to lastIndexOf alone is not
sufficient ). Or is your assumption that the IPv6 literal is
not enclosed in square brackets?


More specifically, what syntax are you proposing to pass IPv6
addresses from the command line?



For JDI connectors (SocketTransportService.java) there are 2 separate 
connector arguments: "localAddress" and "port".
For JDWP agent (socketTransport.c) arguments are provided as a single 
command line argument:
java 
-agentlib:jdwp=transport=dt_socket,server=y,address=: MyApp


--alex


-Chris

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

2019-04-01 Thread Alex Menkov 

Hi Chris,

On 04/01/2019 04:50, Chris Hegarty wrote:

Alex,

On 29/03/2019 22:07, Alex Menkov wrote:

(added net-dev as suggested by Alan)

Net gurus, please assist in reviewing socket-related code.

New webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.01/


Specifically on SocketTransportService.java. What Arthur has
proposed is better ( changing to lastIndexOf alone is not
sufficient ). Or is your assumption that the IPv6 literal is
not enclosed in square brackets?


I didn't know about enclosing IPv6 in square brackets, but looks like 
that's standard way to alleviate conflict between IPv6 address and colon 
as port separator.
Will update the fix to handle them in both JDI connectors 
(SocketTransportService.java) and debugger agent (socketTransport.c)




If keeping Arthur's `static class HostPort` please make the
fields final.

 >> Would it make sense to support the preference properties?
 >>    java.net.preferIPv4Stack
 >>    java.net.preferIPv6Addresses

I'm not sure about this, especially given the property name
prefixes. I need to think a little more on it.


In the initial version of the fix I didn't check the properties.
The rationale here is backward compatibility - is address is empty, old 
debuggers tries to connect to IPv4 address only.
But handling this properties we will better handle clients with 
properties set (as jdb or any other debugger which uses JDI connectors 
are affected by the properties).


BTW fix provided by Arthur implements listening on localhost differently 
- it creates several sockets and binds them to both IPv4 and IPv6 
addresses. But the problem here (and that's the reason I decide to not 
implement it this way) - how to handle the case when we successfully 
bind on one address (for example IPv4), but fail to bind on other (for 
example the port is busy for IPv6 stack). Arthur's version just fail in 
the case (i.e. the whole Java process terminates) and I don't think this 
is good behavior.





There is quite a bit of new native code, is it possible to
rewrite any of this in Java, e.g. reading of the system
properties ( if that is to remain )?


socketTransport.c is a debugger agent which is completely native.

--alex



-Chris.

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

2019-04-01 Thread Alex Menkov 

Hi Daniel, Chris,

Unfortunately docs are out-dated (I plan to update it in JDK13).
In JDK9
https://bugs.openjdk.java.net/browse/JDK-8041435
changed the behavior - empty address (i.e. only port is specified) means 
"local connections only", "*" means "all available connections".

Then in JDK10 new "allow" option was introduced:
https://bugs.openjdk.java.net/browse/JDK-8061228
(it allows to specify list of addresses/subnets to accept connections from).

--alex

On 04/01/2019 06:40, Chris Hegarty wrote:


On 01/04/2019 10:51, Daniel Fuchs wrote:

Hi Arthur,

Not directly related to Alex's original question but...

On 30/03/2019 00:03, Arthur Eubanks wrote:
We have some ipv6 patches as well in this area as well (as well as 
other patches to support ipv6 only environments) that we're trying to 
upstream. I don't understand the code myself, but it might be useful 
to take a look:

http://cr.openjdk.java.net/~aeubanks/jdwpipv6/webrev.00/index.html


SocketTransportService.java:

On my machine at least, InetAddress.getByName("localhost") and
InetAddress.getLocalHost() are quite different:


InetAddress.getByName("localhost") will return the loopback (127.0.0.1)
InetAddress.getLocalHost() returns the local hostname (dhcp-XXX-XXX...)

If I'm not mistaken your changes in HostPort would transform something
that previously returned the loopback (no host specified) with something
that returns the local host name ("*" specified).


So I'm not sure these changes are quite right.
Maybe Alex will be able to confirm.


We need to consult the documentation for the command-line tools that
exercise this code, or the higher-level API spec for the expected
behavior of `*`, port-only specified, etc.

-Chris.

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

2019-04-01 Thread Chris Hegarty 



On 01/04/2019 10:51, Daniel Fuchs wrote:

Hi Arthur,

Not directly related to Alex's original question but...

On 30/03/2019 00:03, Arthur Eubanks wrote:
We have some ipv6 patches as well in this area as well (as well as 
other patches to support ipv6 only environments) that we're trying to 
upstream. I don't understand the code myself, but it might be useful 
to take a look:

http://cr.openjdk.java.net/~aeubanks/jdwpipv6/webrev.00/index.html


SocketTransportService.java:

On my machine at least, InetAddress.getByName("localhost") and
InetAddress.getLocalHost() are quite different:


InetAddress.getByName("localhost") will return the loopback (127.0.0.1)
InetAddress.getLocalHost() returns the local hostname (dhcp-XXX-XXX...)

If I'm not mistaken your changes in HostPort would transform something
that previously returned the loopback (no host specified) with something
that returns the local host name ("*" specified).


So I'm not sure these changes are quite right.
Maybe Alex will be able to confirm.


We need to consult the documentation for the command-line tools that
exercise this code, or the higher-level API spec for the expected
behavior of `*`, port-only specified, etc.

-Chris.

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

2019-04-01 Thread Chris Hegarty 




On 01/04/2019 12:50, Chris Hegarty wrote:

...
Specifically on SocketTransportService.java. What Arthur has
proposed is better ( changing to lastIndexOf alone is not
sufficient ). Or is your assumption that the IPv6 literal is
not enclosed in square brackets?


More specifically, what syntax are you proposing to pass IPv6
addresses from the command line?

-Chris

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

2019-04-01 Thread Chris Hegarty 

Alex,

On 29/03/2019 22:07, Alex Menkov wrote:

(added net-dev as suggested by Alan)

Net gurus, please assist in reviewing socket-related code.

New webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.01/


Specifically on SocketTransportService.java. What Arthur has
proposed is better ( changing to lastIndexOf alone is not
sufficient ). Or is your assumption that the IPv6 literal is
not enclosed in square brackets?

If keeping Arthur's `static class HostPort` please make the
fields final.

>> Would it make sense to support the preference properties?
>>java.net.preferIPv4Stack
>>java.net.preferIPv6Addresses

I'm not sure about this, especially given the property name
prefixes. I need to think a little more on it.

There is quite a bit of new native code, is it possible to
rewrite any of this in Java, e.g. reading of the system
properties ( if that is to remain )?

-Chris.

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

2019-04-01 Thread Daniel Fuchs 

Hi Arthur,

Not directly related to Alex's original question but...

On 30/03/2019 00:03, Arthur Eubanks wrote:
We have some ipv6 patches as well in this area as well (as well as other 
patches to support ipv6 only environments) that we're trying to 
upstream. I don't understand the code myself, but it might be useful to 
take a look:

http://cr.openjdk.java.net/~aeubanks/jdwpipv6/webrev.00/index.html


SocketTransportService.java:

On my machine at least, InetAddress.getByName("localhost") and
InetAddress.getLocalHost() are quite different:


InetAddress.getByName("localhost") will return the loopback (127.0.0.1)
InetAddress.getLocalHost() returns the local hostname (dhcp-XXX-XXX...)

If I'm not mistaken your changes in HostPort would transform something
that previously returned the loopback (no host specified) with something
that returns the local host name ("*" specified).


So I'm not sure these changes are quite right.
Maybe Alex will be able to confirm.

best regards,

-- daniel

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

2019-03-29 Thread Alex Menkov 

(added net-dev as suggested by Alan)

Net gurus, please assist in reviewing socket-related code.

New webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.01/


On 03/28/2019 07:44, Gary Adams wrote:
Is there any documentation that needs to be updated along with the impl 
changes?


created an issue for this:
https://bugs.openjdk.java.net/browse/JDK-8221707



Would it make sense to support the preference properties?
   java.net.preferIPv4Stack
   java.net.preferIPv6Addresses


Done (with new test for the functionality)



Will the previous jdwp tests run with IPv6 or just the new additions?


many tests use "localhost" (or empty string which has the same meaning) 
address. localhost can be resolved to IPv4 or IPv6 address, but by 
default IPv4 is used.

So IPv6 will be used only on IPv6-only systems.



Should probably have reviewers with networking expertise. core-libs(?)

Do we know if IPv6 is enabled in our test infrastructure?


Accordingly the logs IPv6 stack is enabled, but no external IPv6 address 
is assigned (i.e. the only IPv6 address is loopback - ::1). This is 
enough for testing.




A stray "printf" statement in the initial webrev. socketTransport.c


fixed.

--alex



On 3/27/19, 7:04 PM, Alex Menkov wrote:

Hi all,

Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8184770
webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.00/

Main changes are in socketTransport.c - the code is updated to support 
both IPv4 and IPv6.

Some details to simplify reviewing:
- listening:
  - if IP address is specified (like 127.0.0.1 or ::1), connector 
listens only on this address;
  - for backward compatibility if no address (or "localhost") is 
specified, IPv4 is used (if supported by the host);
  - if "*" is specified (means "listen on all local interfaces"), dual 
mode socket is used to listen on both IPv6 and IPv4 addresses;
  - AllowedPeerInfo structure (for "allow" option) is updated to use 
IPv6 address/mask, support for IPv4 is implemented by using "mapped" 
IPv4 addresses;
- attaching: agent resolves and tries to connect to all (IPv4 and 
IPv6) addresses, IPv4 are tried first;


SocketListeningConnector.java/SocketTransportService.java are updated 
to support IPv6 addresses (the addresses may contain colons);


new JdwpAttachTest.java/JdwpListenTest.java test that listening and 
attaching works for all available addresses (Ipv4 and IPv6)


BasicJDWPConnectionTest.java was renamed to JdwpAllowTest.java (as it 
tests "allow" functionality), tests for mask (prefix length) 
functionality are added (for both IPv4 and IPv6);


--alex

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

2019-03-28 Thread Alan Bateman 

On 28/03/2019 14:44, Gary Adams wrote:
Is there any documentation that needs to be updated along with the 
impl changes?


Would it make sense to support the preference properties?
  java.net.preferIPv4Stack
  java.net.preferIPv6Addresses

Will the previous jdwp tests run with IPv6 or just the new additions?

Should probably have reviewers with networking expertise. core-libs(?)
Maybe include net-dev as this is mostly about socket transport rather 
than anything debugger specific.


-Alan

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

2019-03-28 Thread Gary Adams 
Is there any documentation that needs to be updated along with the impl 
changes?


Would it make sense to support the preference properties?
  java.net.preferIPv4Stack
  java.net.preferIPv6Addresses

Will the previous jdwp tests run with IPv6 or just the new additions?

Should probably have reviewers with networking expertise. core-libs(?)

Do we know if IPv6 is enabled in our test infrastructure?

A stray "printf" statement in the initial webrev. socketTransport.c

On 3/27/19, 7:04 PM, Alex Menkov wrote:

Hi all,

Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8184770
webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.00/

Main changes are in socketTransport.c - the code is updated to support 
both IPv4 and IPv6.

Some details to simplify reviewing:
- listening:
  - if IP address is specified (like 127.0.0.1 or ::1), connector 
listens only on this address;
  - for backward compatibility if no address (or "localhost") is 
specified, IPv4 is used (if supported by the host);
  - if "*" is specified (means "listen on all local interfaces"), dual 
mode socket is used to listen on both IPv6 and IPv4 addresses;
  - AllowedPeerInfo structure (for "allow" option) is updated to use 
IPv6 address/mask, support for IPv4 is implemented by using "mapped" 
IPv4 addresses;
- attaching: agent resolves and tries to connect to all (IPv4 and 
IPv6) addresses, IPv4 are tried first;


SocketListeningConnector.java/SocketTransportService.java are updated 
to support IPv6 addresses (the addresses may contain colons);


new JdwpAttachTest.java/JdwpListenTest.java test that listening and 
attaching works for all available addresses (Ipv4 and IPv6)


BasicJDWPConnectionTest.java was renamed to JdwpAllowTest.java (as it 
tests "allow" functionality), tests for mask (prefix length) 
functionality are added (for both IPv4 and IPv6);


--alex

RFR: JDK-8184770: JDWP support for IPv6

2019-03-27 Thread Alex Menkov 

Hi all,

Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8184770
webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.00/

Main changes are in socketTransport.c - the code is updated to support 
both IPv4 and IPv6.

Some details to simplify reviewing:
- listening:
  - if IP address is specified (like 127.0.0.1 or ::1), connector 
listens only on this address;
  - for backward compatibility if no address (or "localhost") is 
specified, IPv4 is used (if supported by the host);
  - if "*" is specified (means "listen on all local interfaces"), dual 
mode socket is used to listen on both IPv6 and IPv4 addresses;
  - AllowedPeerInfo structure (for "allow" option) is updated to use 
IPv6 address/mask, support for IPv4 is implemented by using "mapped" 
IPv4 addresses;
- attaching: agent resolves and tries to connect to all (IPv4 and IPv6) 
addresses, IPv4 are tried first;


SocketListeningConnector.java/SocketTransportService.java are updated to 
support IPv6 addresses (the addresses may contain colons);


new JdwpAttachTest.java/JdwpListenTest.java test that listening and 
attaching works for all available addresses (Ipv4 and IPv6)


BasicJDWPConnectionTest.java was renamed to JdwpAllowTest.java (as it 
tests "allow" functionality), tests for mask (prefix length) 
functionality are added (for both IPv4 and IPv6);


--alex