> 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?). 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
