RFR [XS]: 8210147: adjust some WSAGetLastError usages in windows network coding - was : RE: WSAGetLastError usage in jdk/src/java.base/windows/native/libnet/SocketInputStream.c
Baesken, Matthias
matthias.baesken at sap.com
Wed Aug 29 13:25:12 UTC 2018
Thanks Thomas !
Can I have a second review please ?
Best regards, Matthias
> -----Original Message-----
> From: Thomas Stüfe <thomas.stuefe at gmail.com>
> Sent: Mittwoch, 29. August 2018 15:22
> To: Baesken, Matthias <matthias.baesken at sap.com>
> Cc: net-dev <net-dev at openjdk.java.net>
> Subject: Re: RFR [XS]: 8210147: adjust some WSAGetLastError usages in
> windows network coding - was : RE: WSAGetLastError usage in
> jdk/src/java.base/windows/native/libnet/SocketInputStream.c
>
> Looks fine.
>
> Regards, Thomas
>
> On Wed, Aug 29, 2018 at 3:15 PM, Baesken, Matthias
> <matthias.baesken at 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.stuefe at gmail.com>
> >> Sent: Mittwoch, 29. August 2018 11:22
> >> To: Baesken, Matthias <matthias.baesken at sap.com>
> >> Cc: net-dev <net-dev at 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.baesken at 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.stuefe at gmail.com>
> >> >> Sent: Dienstag, 28. August 2018 18:33
> >> >> To: Baesken, Matthias <matthias.baesken at sap.com>
> >> >> Cc: net-dev <net-dev at 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.baesken at 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.stuefe at gmail.com>
> >> >> >> Sent: Dienstag, 28. August 2018 14:23
> >> >> >> To: Baesken, Matthias <matthias.baesken at sap.com>
> >> >> >> Cc: net-dev <net-dev at 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.baesken at 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
More information about the net-dev
mailing list