Looks fine. Regards, Thomas
On Wed, Aug 29, 2018 at 3:15 PM, Baesken, Matthias <matthias.baes...@sap.com> wrote: > Please review this change, it adjusts some usages of WSAGetLastError > in windows net - coding. > > Change : > > http://cr.openjdk.java.net/~mbaesken/webrevs/8210147/ > > Bug : > > https://bugs.openjdk.java.net/browse/JDK-8210147 > > Thanks , Matthias > > > >> -----Original Message----- >> From: Thomas Stüfe <thomas.stu...@gmail.com> >> Sent: Mittwoch, 29. August 2018 11:22 >> To: Baesken, Matthias <matthias.baes...@sap.com> >> Cc: net-dev <net-dev@openjdk.java.net> >> Subject: Re: 8210147: adjust some WSAGetLastError usages in windows >> network coding - was : RE: WSAGetLastError usage in >> jdk/src/java.base/windows/native/libnet/SocketInputStream.c >> >> Hi Matthias, >> >> On Wed, Aug 29, 2018 at 9:46 AM, Baesken, Matthias >> <matthias.baes...@sap.com> wrote: >> > >> >> What I would do to keep the change simple is just passing 0 or -1 as >> >> errorNum argument to NET_ThrowNew - in that case it will say >> >> "Unspecified socket error" in the exception. Just make sure that 0 or >> >> -1, whatever you use, is not one of the WSAxxxxx constants >> > >> > Hi Thomas, according to >> > >> > https://docs.microsoft.com/en-us/windows/desktop/winsock/windows- >> sockets-error-codes-2 >> > >> > both 0 and -1 are fine . >> > >> > Instead of using >> > >> > NET_ThrowNew(env, -1, "Unable to allocate memory"); >> > >> > We could also call : >> > >> > JNU_ThrowOutOfMemoryError(env, "Native heap allocation failed"); >> > >> > (or would this be problematic because of compatibility?). >> > >> > >> >> That should be fine too. >> >> Best Regards, Thomas >> >> > >> > Btw I opened a bug : >> > >> > https://bugs.openjdk.java.net/browse/JDK-8210147 >> > >> > for the issues . >> > >> > Best regards, Matthias >> > >> > >> > >> >> -----Original Message----- >> >> From: Thomas Stüfe <thomas.stu...@gmail.com> >> >> Sent: Dienstag, 28. August 2018 18:33 >> >> To: Baesken, Matthias <matthias.baes...@sap.com> >> >> Cc: net-dev <net-dev@openjdk.java.net> >> >> Subject: Re: WSAGetLastError usage in >> >> jdk/src/java.base/windows/native/libnet/SocketInputStream.c >> >> >> >> On Tue, Aug 28, 2018 at 5:56 PM, Baesken, Matthias >> >> <matthias.baes...@sap.com> wrote: >> >> >> not strictly necessary, since WSAGetLastError() only refers to socket >> calls >> >> > >> >> > I'll try to find out more about this. >> >> > >> >> >> However, I think your proposal is fine for code cleanliness sake. >> >> > >> >> > Yes , then I will do it for code cleanliness sake (and because MS >> >> recommends anyway to get the error immediately). >> >> > >> >> > When looking at the WSAGetLastError() usages in the whole JDK >> >> Windows code I found 2 strange usages , >> >> > Both times it looks like WSAGetLastError() is used to get the >> >> > error of >> >> the malloc call , in case this really works, >> >> > WSAGetLastError might indeed alias in some way to GetLastError : >> >> > >> >> > jdk/src/java.base/windows/native/libnet/Inet6AddressImpl.c >> >> > >> >> > 384static jboolean >> >> > 385ping6(JNIEnv *env, HANDLE hIcmpFile, SOCKETADDRESS *sa, >> >> > 386 SOCKETADDRESS *netif, jint timeout) >> >> > 387{ >> >> > .............. >> >> > 396 ReplyBuffer = (VOID *)malloc(ReplySize); >> >> > 397 if (ReplyBuffer == NULL) { >> >> > 398 IcmpCloseHandle(hIcmpFile); >> >> > 399 NET_ThrowNew(env, WSAGetLastError(), "Unable to allocate >> >> memory"); >> >> > 400 return JNI_FALSE; >> >> > 401 } >> >> > >> >> >> >> Thats plain wrong. >> >> >> >> One should use errno instead, however, the Windows variant of >> >> NET_ThrowNew expects a WSA error code for the error number, it does >> >> not handle errno. >> >> >> >> What I would do to keep the change simple is just passing 0 or -1 as >> >> errorNum argument to NET_ThrowNew - in that case it will say >> >> "Unspecified socket error" in the exception. Just make sure that 0 or >> >> -1, whatever you use, is not one of the WSAxxxxx constants, see >> >> winsock_errors[] table in net_util_md.c. >> >> >> >> > >> >> > jdk/src/java.base/windows/native/libnet/Inet4AddressImpl.c >> >> > >> >> > 306static jboolean >> >> > 307ping4(JNIEnv *env, HANDLE hIcmpFile, SOCKETADDRESS *sa, >> >> > 308 SOCKETADDRESS *netif, jint timeout) >> >> > 309{ >> >> > ......... >> >> > 326 ReplyBuffer = (VOID *)malloc(ReplySize); >> >> > 327 if (ReplyBuffer == NULL) { >> >> > 328 IcmpCloseHandle(hIcmpFile); >> >> > 329 NET_ThrowNew(env, WSAGetLastError(), "Unable to allocate >> >> memory"); >> >> > 330 return JNI_FALSE; >> >> > 331 } >> >> > >> >> > >> >> >> >> Yes, this may be wrong if WSAGetLastError() uses GetLastError() >> >> internally, so I would move the WSAGetLastError() call up. >> >> >> >> Good catches! >> >> >> >> ..Thomas >> >> >> >> > Best regards, Matthias >> >> > >> >> > >> >> > >> >> > >> >> >> -----Original Message----- >> >> >> From: Thomas Stüfe <thomas.stu...@gmail.com> >> >> >> Sent: Dienstag, 28. August 2018 14:23 >> >> >> To: Baesken, Matthias <matthias.baes...@sap.com> >> >> >> Cc: net-dev <net-dev@openjdk.java.net> >> >> >> Subject: Re: WSAGetLastError usage in >> >> >> jdk/src/java.base/windows/native/libnet/SocketInputStream.c >> >> >> >> >> >> Hi Matthias, >> >> >> >> >> >> not strictly necessary, since WSAGetLastError() only refers to socket >> >> >> calls and GetIntField() is unlikely to call socket functions... >> >> >> However, I think your proposal is fine for code cleanliness sake. >> >> >> >> >> >> Best Regards, Thomas >> >> >> >> >> >> On Tue, Aug 28, 2018 at 2:13 PM, Baesken, Matthias >> >> >> <matthias.baes...@sap.com> wrote: >> >> >> > Hello, >> >> >> > >> >> >> > the MSDN docu about WSAGetLastError warns to get the error-code >> >> >> > ***immediately*** after occurance. >> >> >> > >> >> >> > See : >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://msdn.microsoft.com/de- >> >> >> de/library/windows/desktop/ms741580(v=vs.85).aspx >> >> >> > >> >> >> > >> >> >> > >> >> >> > " ... If a function call's return value indicates that error or other >> >> >> > relevant data was returned in the error code, >> >> >> > >> >> >> > WSAGetLastError should be called immediately ..." >> >> >> > >> >> >> > >> >> >> > >> >> >> > However in windows SocketInputStream.c , this was not done; we >> >> noticed >> >> >> very >> >> >> > seldom errors because of this (not reproducible however) so >> >> >> > >> >> >> > we had a fix for this in our code base for a long time. >> >> >> > >> >> >> > >> >> >> > >> >> >> > Should we change this as well in OpenJDK , for example from : >> >> >> > >> >> >> > >> >> >> > >> >> >> > jdk/src/java.base/windows/native/libnet/SocketInputStream.c >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > 120 nread = recv(fd, bufP, len, 0); >> >> >> > >> >> >> > 121 if (nread > 0) { >> >> >> > >> >> >> > 122 (*env)->SetByteArrayRegion(env, data, off, nread, (jbyte >> >> *)bufP); >> >> >> > >> >> >> > 123 } else { >> >> >> > >> >> >> > 124 if (nread < 0) { >> >> >> > >> >> >> > 125 // Check if the socket has been closed since we last >> checked. >> >> >> > >> >> >> > 126 // This could be a reason for recv failing. >> >> >> > >> >> >> > 127 if ((*env)->GetIntField(env, fdObj, IO_fd_fdID) == >> >> >> > -1) { >> >> >> > >> >> >> > 128 JNU_ThrowByName(env, "java/net/SocketException", >> >> "Socket >> >> >> > closed"); >> >> >> > >> >> >> > 129 } else { >> >> >> > >> >> >> > 130 switch (WSAGetLastError()) { >> >> >> > >> >> >> > >> >> >> > >> >> >> > to : >> >> >> > >> >> >> > >> >> >> > >> >> >> > 120 nread = recv(fd, bufP, len, 0); >> >> >> > >> >> >> > 121 if (nread > 0) { >> >> >> > >> >> >> > 122 (*env)->SetByteArrayRegion(env, data, off, nread, (jbyte >> >> *)bufP); >> >> >> > >> >> >> > 123 } else { >> >> >> > >> >> >> > 124 if (nread < 0) { >> >> >> > >> >> >> > 125 int err = WSAGetLastError(); >> >> >> > >> >> >> > ... >> >> >> > >> >> >> > switch (err) { >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > Thanks and best regards, Matthias
