Re: JDK-8080990: libdt_socket/socket_md.c(202) : warning C4996: 'gethostbyname': Use getaddrinfo() or GetAddrInfoW()

2018-02-01 Thread gary.ad...@oracle.com 

First pass over the code I disabled the compilation flag and then
did quick substitution for the easier functions. I commented out the
WSASendDisconnect calls so I could see what tests would fail if
the function was just removed. I have a replacement now that uses
"shutdown(fd,SD_SEND)", but I still have a few more test failures to
investigate.

I updated the error message text for "getaddrinfo".

I'll post a revised webrev after the 4 jshell errors are corrected.


On 2/1/18 3:11 AM, Langer, Christoph wrote:

Hi Gary,

I was having a look at your changes.

I'm wondering what the reason is behind uncommenting WSASendDisconnect in 
Java_sun_nio_ch_SocketDispatcher_preClose0 of file SocketDispatcher.c? And in 
dbgsysSocketClose?

In socketTransport.c, line:
 331 setLastError(0, "gethostbyname: unknown host");
you should change the description text of the error to getaddrinfo instead of 
gethostbyname.

Best regards
Christoph



-Original Message-
From: build-dev [mailto:[email protected]] On Behalf Of Gary 
Adams
Sent: Donnerstag, 25. Januar 2018 19:47
To: OpenJDK Serviceability ; OpenJDK Build 
; OpenJDK Networking 
Subject: RFR: JDK-8080990: libdt_socket/socket_md.c(202) : warning C4996: 
'gethostbyname': Use getaddrinfo() or GetAddrInfoW()

Here's a first pass attempt to remove the -D_WINSOCK_DEPRECATED_NO_WARNINGS
build flag and update the winsock deprecated functions.

Issue: https://bugs.openjdk.java.net/browse/JDK-8080990
Webrev: http://cr.openjdk.java.net/~gadams/8080990/

These are the initial deprecated functions updated:
 gethostbyname -> getaddrinfo
 inet_addr -> inet_pton
 inet_ntoa -> inet_ntop

I'm not sure how to replace the existing WSASendDisconnect calls.
It's not clear that it actually provides a graceful disconnect.

Re: JDK-8080990: libdt_socket/socket_md.c(202) : warning C4996: 'gethostbyname': Use getaddrinfo() or GetAddrInfoW()

2018-02-07 Thread gary.ad...@oracle.com 

On 2/7/18 12:31 PM, Chris Hegarty wrote:

Gary,


http://cr.openjdk.java.net/%7Egadams/8080990/webrev.02/

I think the replacement of WSASendDisconnect with
shutdown(SD_SEND) should be fine. I do note that there
is another usage of WSASendDisconnect in
java.base/windows/native/libnet/net_util_md.c.

Thanks for the reference.
I'm not sure how that one slipped by unnoticed.


[ Maybe you want to separate out the changes in java.base
( NIO and NET ) from the serviceability changes? Up to
you. ]

Actually, this change  is about removing the compilation flag globally
in flags.m4 and any sources that need to be updated
to replace the deprecated functions.


Curious about the specific hints you have chosen to use.
In other areas we have the following:
   hints.ai_flags = AI_CANONNAME;
   hints.ai_family = AF_INET;

[ Not saying that what you have is incorrect, just questioning
  if you need to specify the socket type and protocol ]

One of the benefits of getaddrinfo is that it can return a list of
addresses from the name service. In the places we were using
gethostbyname we were looking for just one ipv4 address.
By adding additional constraints on the supplied hints, it can help
reduce the list of returned addresses.



-Chris.

Re: JDK-8080990: libdt_socket/socket_md.c(202) : warning C4996: 'gethostbyname': Use getaddrinfo() or GetAddrInfoW()

2018-02-07 Thread gary.ad...@oracle.com 

On 2/7/18 12:48 PM, Alan Bateman wrote:

On 07/02/2018 17:31, Chris Hegarty wrote:

Gary,


http://cr.openjdk.java.net/%7Egadams/8080990/webrev.02/

I think the replacement of WSASendDisconnect with
shutdown(SD_SEND) should be fine. I do note that there
is another usage of WSASendDisconnect in
java.base/windows/native/libnet/net_util_md.c.

[ Maybe you want to separate out the changes in java.base
( NIO and NET ) from the serviceability changes? Up to
you. ]

Is NIO SocketDispatcher change needed in this patch? I would prefer if 
we could separate this was WSASendDisconnect was the semantics that we 
need.


-Alan

Yes, WSASendDisconnect is deprecated in vs2013.
As far as I know "shutdown(fd, SD_SEND)" prevents further outgoing writes
and there was no final payload to send.

I have not seen any failures in the java/nio tests.

Re: JDK-8080990: libdt_socket/socket_md.c(202) : warning C4996: 'gethostbyname': Use getaddrinfo() or GetAddrInfoW()

2018-02-07 Thread gary.ad...@oracle.com 

On 2/7/18 12:55 PM, [email protected] wrote:

On 2/7/18 12:31 PM, Chris Hegarty wrote:

Gary,


http://cr.openjdk.java.net/%7Egadams/8080990/webrev.02/

I think the replacement of WSASendDisconnect with
shutdown(SD_SEND) should be fine. I do note that there
is another usage of WSASendDisconnect in
java.base/windows/native/libnet/net_util_md.c.

Thanks for the reference.
I'm not sure how that one slipped by unnoticed.

I found in make/lib/NetworkingLibraries.gmk the BUILD_LIBNET
target includes a number of specific disabled warnings. In particular,
4996 which suppresses all deprecated function warnings.

I think I'd like to get the current changes pushed to remove the specific
WINSOCK_DEPRECATED_NO_WARNINGS and then file another bug to
come back and look at selective makefiles that use a blanket 4996
suppression of deprecated warnings.



[ Maybe you want to separate out the changes in java.base
( NIO and NET ) from the serviceability changes? Up to
you. ]

Actually, this change  is about removing the compilation flag globally
in flags.m4 and any sources that need to be updated
to replace the deprecated functions.


Curious about the specific hints you have chosen to use.
In other areas we have the following:
   hints.ai_flags = AI_CANONNAME;
   hints.ai_family = AF_INET;

[ Not saying that what you have is incorrect, just questioning
  if you need to specify the socket type and protocol ]

One of the benefits of getaddrinfo is that it can return a list of
addresses from the name service. In the places we were using
gethostbyname we were looking for just one ipv4 address.
By adding additional constraints on the supplied hints, it can help
reduce the list of returned addresses.



-Chris.

Re: JDK-8080990: libdt_socket/socket_md.c(202) : warning C4996: 'gethostbyname': Use getaddrinfo() or GetAddrInfoW()

2018-02-07 Thread gary.ad...@oracle.com 

A fresh webrev rebased with latest jdk/jdk repos:
 http://cr.openjdk.java.net/~gadams/8080990/webrev.03/index.html

If there are no more comments, I'll check in locally with these reviewers
  clanger,chegar,erikj

and cut patches on Thurs AM.


On 2/5/18 2:15 PM, Gary Adams wrote:

One more to fix to cover the remaining test failures I was seeing.

Previously, using inet_addr, there was a single -1 return that needed 
to be checked.
Now that we're using inet_pton, there is a -1 and a 0 error return to 
be considered.


My preference would be to leave the dbgSysInetAddr name unchanged and 
have the low level
check for inet_pton errors to simply return the -1 that was previously 
checked.


This specific issue is about the deprecation of library functions on 
windows. I'd recommend not
upgrading the other platforms at this time until a complete update is 
done for ipv6 support.


I'll post a new webrev when I've retested using the updated inet_pton 
error checking.


...

Hi Gary,


>/Here's a revised webrev />//>/http://cr.openjdk.java.net/~gadams/8080990/webrev.01/index.html 
 />//>/Still testing ... />//>/Using shutdown() fixed problems reported by the />/java/nio/channelSocketChannel tests. /

The fix looks good. I would think we should rename dbgsysInetAddr to dbgsysPToN 
and use inet_pton also for the Unix/Linux platforms. It would be the better 
choice there as well, though we still only support IPv4 in libdt_socket.

>//>/I also noticed prior use of getaddrinfo for "localhost" was not calling 
/>/freeaddrinfo. /
Thanks for spotting/fixing that.

Best regards
Christoph