RFR 8043954: Fix behavior difference of connect() for AIX
Hello, Could I have following patch reviewed for bug 8034954 ? http://cr.openjdk.java.net/~luchsh/JDK-8043954/ The patch is to fix a behavior difference of connect() API for AIX platform, according to the documentation, http://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.aix.commtrf... On AIX, when connect() got interrupted by signal, the underlying connection will be made asynchronously, "EINTR The attempt to establish a connection was interrupted by delivery of a signal that was caught; the connection will be established asynchronously." This fix tries to poll() for successfully established connection or error in the similar way as NET_Timeout(). Thanks Jonathan
Hi Jonathan, thanks for fixing this! I've looked at the change and it looks good to me (but I'm not a reviewer). The only minor flaw I found is that you declare the helper variable 'int rc = -1' but never assign it. Instead you could just return '-1' directly where you currently return 'rc' and remove 'rc' altogether. I'm currently still doing test build and I'll run some tests. I'll let you know if I should see any problems. By the way - does this change fix a real problem or is it just an improvement of the current implementation (just curious)? Thank you and best regards, Volker On Tue, Jun 3, 2014 at 11:51 AM, Jonathan Lu <luchsh@linux.vnet.ibm.com> wrote:
Hello,
Could I have following patch reviewed for bug 8034954 ?
http://cr.openjdk.java.net/~luchsh/JDK-8043954/
The patch is to fix a behavior difference of connect() API for AIX platform, according to the documentation, http://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.aix.commtrf...
On AIX, when connect() got interrupted by signal, the underlying connection will be made asynchronously,
"EINTR The attempt to establish a connection was interrupted by delivery of a signal that was caught; the connection will be established asynchronously."
This fix tries to poll() for successfully established connection or error in the similar way as NET_Timeout().
Thanks Jonathan
Hi Volker, Thanks for your comment! an updated webrev was made at http://cr.openjdk.java.net/~luchsh/JDK-8043954.2/ On Tue, Jun 3, 2014 at 8:48 PM, Volker Simonis <volker.simonis@gmail.com> wrote:
Hi Jonathan,
thanks for fixing this! I've looked at the change and it looks good to me (but I'm not a reviewer). The only minor flaw I found is that you declare the helper variable 'int rc = -1' but never assign it. Instead you could just return '-1' directly where you currently return 'rc' and remove 'rc' altogether.
The new patch contains this change and another formatting change.
I'm currently still doing test build and I'll run some tests. I'll let you know if I should see any problems.
By the way - does this change fix a real problem or is it just an improvement of the current implementation (just curious)?
The change is now an improvement.
Thank you and best regards, Volker
On Tue, Jun 3, 2014 at 11:51 AM, Jonathan Lu <luchsh@linux.vnet.ibm.com> wrote:
Hello,
Could I have following patch reviewed for bug 8034954 ?
http://cr.openjdk.java.net/~luchsh/JDK-8043954/
The patch is to fix a behavior difference of connect() API for AIX platform, according to the documentation,
http://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.aix.commtrf...
On AIX, when connect() got interrupted by signal, the underlying
connection
will be made asynchronously,
"EINTR The attempt to establish a connection was interrupted by delivery of a signal that was caught; the connection will be established asynchronously."
This fix tries to poll() for successfully established connection or error in the similar way as NET_Timeout().
Thanks Jonathan
Thank you and best regards Jonathan
On 04/06/2014 07:31, Jonathan Lu wrote:
Hi Volker,
Thanks for your comment! an updated webrev was made at http://cr.openjdk.java.net/~luchsh/JDK-8043954.2/
Would it make sense to set errno to sockopt_arg for the case that getsockopt(SO_ERROR) returns an error? Otherwise it looks okay to me. -Alan.
Hi Alan, On Wed, Jun 4, 2014 at 9:16 PM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 04/06/2014 07:31, Jonathan Lu wrote:
Hi Volker,
Thanks for your comment! an updated webrev was made at http://cr.openjdk.java.net/~luchsh/JDK-8043954.2/
Would it make sense to set errno to sockopt_arg for the case that getsockopt(SO_ERROR) returns an error? Otherwise it looks okay to me.
If getsockopt(SO_ERROR) failed, I did not find any explicit docs about the behavior. but as I tested with some C code snippet, the value of sockopt_arg would not be changed if getsockopt(SO_ERROR) failed. So I prefer to keep the current approach, does it make sense to you ?
-Alan.
Many thanks Jonathan
On 05/06/2014 11:37, Jonathan Lu wrote:
If getsockopt(SO_ERROR) failed, I did not find any explicit docs about the behavior. but as I tested with some C code snippet, the value of sockopt_arg would not be changed if getsockopt(SO_ERROR) failed. So I prefer to keep the current approach, does it make sense to you ?
The case that I was wondering about is the common case where getsockopt(SO_ERROR) succeeds and I was wondering if the code should actually be: if (sockopt_arg != 0 ) { errno = sockopt_arg; return -1; } That way the caller of NET_Connect will have errno set so that XXX_ThrowByNameWithLastError can create an appropriate exception message. -Alan.
Hi Alan, On Fri, Jun 6, 2014 at 1:53 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 05/06/2014 11:37, Jonathan Lu wrote:
If getsockopt(SO_ERROR) failed, I did not find any explicit docs about the behavior. but as I tested with some C code snippet, the value of sockopt_arg would not be changed if getsockopt(SO_ERROR) failed. So I prefer to keep the current approach, does it make sense to you ?
The case that I was wondering about is the common case where getsockopt(SO_ERROR) succeeds and I was wondering if the code should actually be:
if (sockopt_arg != 0 ) { errno = sockopt_arg; return -1; }
That way the caller of NET_Connect will have errno set so that XXX_ThrowByNameWithLastError can create an appropriate exception message.
You are right! errno will be checked by other code if NET_Connect() failed, I've updated the patch, please help to review. http://cr.openjdk.java.net/~luchsh/JDK-8043954.3/
-Alan.
Many thanks - Jonathan
Hi Jonathan, I just wanted to let you know that I've build your changes on AIX 5..3 and 7.1. I've also run the jdk regression tests without specific issues. So thumbs up from me! Regards, Volker On Fri, Jun 6, 2014 at 12:03 PM, Jonathan Lu <luchsh@linux.vnet.ibm.com> wrote:
Hi Alan,
On Fri, Jun 6, 2014 at 1:53 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 05/06/2014 11:37, Jonathan Lu wrote:
If getsockopt(SO_ERROR) failed, I did not find any explicit docs about the behavior. but as I tested with some C code snippet, the value of sockopt_arg would not be changed if getsockopt(SO_ERROR) failed. So I prefer to keep the current approach, does it make sense to you ?
The case that I was wondering about is the common case where getsockopt(SO_ERROR) succeeds and I was wondering if the code should actually be:
if (sockopt_arg != 0 ) { errno = sockopt_arg; return -1; }
That way the caller of NET_Connect will have errno set so that XXX_ThrowByNameWithLastError can create an appropriate exception message.
You are right! errno will be checked by other code if NET_Connect() failed, I've updated the patch, please help to review.
http://cr.openjdk.java.net/~luchsh/JDK-8043954.3/
-Alan.
Many thanks - Jonathan
Hi Alan, Volker, Thanks a lot! I've pushed the change. Best regards - Jonathan On Fri, Jun 6, 2014 at 8:27 PM, Volker Simonis <volker.simonis@gmail.com> wrote:
Hi Jonathan,
I just wanted to let you know that I've build your changes on AIX 5..3 and 7.1. I've also run the jdk regression tests without specific issues.
So thumbs up from me!
Regards, Volker
On Fri, Jun 6, 2014 at 12:03 PM, Jonathan Lu <luchsh@linux.vnet.ibm.com> wrote:
Hi Alan,
On Fri, Jun 6, 2014 at 1:53 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 05/06/2014 11:37, Jonathan Lu wrote:
If getsockopt(SO_ERROR) failed, I did not find any explicit docs about the behavior. but as I tested with some C code snippet, the value of sockopt_arg would not be changed if getsockopt(SO_ERROR) failed. So I prefer to keep the current approach, does it make sense to you ?
The case that I was wondering about is the common case where getsockopt(SO_ERROR) succeeds and I was wondering if the code should actually be:
if (sockopt_arg != 0 ) { errno = sockopt_arg; return -1; }
That way the caller of NET_Connect will have errno set so that XXX_ThrowByNameWithLastError can create an appropriate exception message.
You are right! errno will be checked by other code if NET_Connect() failed, I've updated the patch, please help to review.
http://cr.openjdk.java.net/~luchsh/JDK-8043954.3/
-Alan.
Many thanks - Jonathan
On 06/06/2014 11:03, Jonathan Lu wrote:
You are right! errno will be checked by other code if NET_Connect() failed, I've updated the patch, please help to review.
http://cr.openjdk.java.net/~luchsh/JDK-8043954.3/ <http://cr.openjdk.java.net/%7Eluchsh/JDK-8043954.3/>
Thanks for the update, this looks good to me now. -Alan.
participants (3)
-
Alan Bateman
-
Jonathan Lu
-
Volker Simonis