Having problems understanding async_connect_send and async_connect_connected in lib/async_req/async_soc.c
Ralph Boehme
rb at sernet.de
Tue Oct 20 15:44:30 UTC 2015
On Sat, Oct 17, 2015 at 07:31:31AM -0700, Richard Sharpe wrote:
> On Fri, Oct 16, 2015 at 11:02 PM, Ralph Boehme <rb at sernet.de> wrote:
> > On Fri, Oct 16, 2015 at 04:55:45PM -0700, Jeremy Allison wrote:
> >> On Fri, Oct 16, 2015 at 11:04:57PM +0200, Ralph Boehme wrote:
> >> > On Fri, Oct 16, 2015 at 01:22:51PM -0700, Richard Sharpe wrote:
> >> > > Hi folks,
> >> > >
> >> > > I am having problems reconciling this man connect with respect to EINPROGRESS:
> >> > >
> >> > > The socket is non-blocking and the connection cannot be com-
> >> > > pleted immediately. It is possible to select(2) or poll(2) for
> >> > > completion by selecting the socket for writing. After select(2)
> >> > > indicates writability, use getsockopt(2) to read the SO_ERROR
> >> > > option at level SOL_SOCKET to determine whether connect() com-
> >> > > pleted successfully (SO_ERROR is zero) or unsuccessfully
> >> > > (SO_ERROR is one of the usual error codes listed here, explain-
> >> > > ing the reason for the failure).
> >> > >
> >> > > However, the code in async_connect_connected simply calls connect
> >> > > again, which strace tells me is getting ECONNREFUSED ...
> >> > >
> >> > > So, I am confused ...
> >> > >
> >> > > Perhaps there is some missing code for the AF_INET case?
> >> >
> >> > at first glance, the code in async_connect_connected seems wrong to
> >> > me. The right thing to do would be calling getsockopt() as described
> >> > in the manpage.
> >> >
> >> > Calling connect() a second time on a non-blocking socket with a
> >> > previous connect in flight would return EALREADY according to man
> >> > socket(2).
> >>
> >> Or EISCONN (according to connect(2)).
> >>
> >> The getsockopt() fix seems best. Do you want to code it
> >> up or shall I ?
> >
> > will do it.
> >
> > -Ralph
>
> I have a suspicion that SeLinux is causing my problem ... so hold off on that.
>
> I coded up a fix that should work but causes the same problem, and a
> perusal of the kernel sources yesterday suggested SeLinux as the
> culprit :-)
couldn't resist, played some more with this. Turns out Solaris *will*
return EISCONN when connect() is called a second time, so imo we have
a bug:
<https://bugzilla.samba.org/show_bug.cgi?id=11564>
Patch attached. I've also crafted a test, which at least ensures basic
functionality of async_connect_send() while not really able to detect
this corner case. Not sure if we want the test in master.
Please review carefully and push if ok. Thanks!
Some references:
- UNIX Network Programming, Volumes 1, by Stevens et al., p.450
- <http://oss.sgi.com/archives/netdev/2002-08/msg00195.html>
- <http://cr.yp.to/docs/connect.html>
- Linux: man 2 connect
-slow
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de,mailto:kontakt@sernet.de
-------------- next part --------------
From 25fbb47e1ec898a8aa165264ffbb14175b250fd5 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sun, 18 Oct 2015 22:23:20 +0200
Subject: [PATCH 1/2] selftest: add a test for async_connect_send()
Bug: https://bugzilla.samba.org/show_bug.cgi?id=11564
Signed-off-by: Ralph Boehme <slow at samba.org>
---
lib/async_req/async_connect_send_test.c | 129 ++++++++++++++++++++++++++++++++
lib/async_req/wscript_build | 4 +
source3/script/tests/test_async_req.sh | 11 +++
source3/selftest/tests.py | 3 +
4 files changed, 147 insertions(+)
create mode 100644 lib/async_req/async_connect_send_test.c
create mode 100755 source3/script/tests/test_async_req.sh
diff --git a/lib/async_req/async_connect_send_test.c b/lib/async_req/async_connect_send_test.c
new file mode 100644
index 0000000..3fe3e61
--- /dev/null
+++ b/lib/async_req/async_connect_send_test.c
@@ -0,0 +1,129 @@
+/*
+ * Test async connect
+ * Copyright (C) Ralph Boehme 2015
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "replace.h"
+#include "lib/tevent/tevent.h"
+#include "lib/async_req/async_sock.h"
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+int main(int argc, const char *argv[])
+{
+ int result, listen_sock, status, exit_status, port;
+ struct sockaddr_in addr = { 0 };
+ pid_t pid;
+
+ listen_sock = socket(PF_INET, SOCK_STREAM, 0);
+ if (listen_sock == -1) {
+ perror("socket() failed");
+ exit(1);
+ }
+
+ addr.sin_family = AF_INET;
+ addr.sin_addr.s_addr = inet_addr("127.0.0.1");
+
+ for (port = 1024; port < INT32_MAX; port++) {
+ addr.sin_port = htons(port);
+ result = bind(listen_sock, (struct sockaddr *)&addr, sizeof(addr));
+ if (result == 0) {
+ break;
+ }
+ }
+
+ if (port == INT32_MAX) {
+ printf("Huh, no free port?\n");
+ return 1;
+ }
+
+ result = listen(listen_sock, 1);
+ if (result == -1) {
+ perror("listen() failed");
+ close(listen_sock);
+ return 1;
+ }
+
+ pid = fork();
+ if (pid == -1) {
+ perror("fork");
+ return 1;
+ }
+
+ if (pid == 0) {
+ struct tevent_context *ev;
+ struct tevent_req *req;
+ int fd;
+
+ ev = tevent_context_init(NULL);
+ if (ev == NULL) {
+ fprintf(stderr, "tevent_context_init failed\n");
+ return 1;
+ }
+
+ fd = socket(PF_INET, SOCK_STREAM, 0);
+ if (fd == -1) {
+ perror("socket");
+ return 1;
+ }
+
+ memset(&addr, sizeof(addr), 0);
+ addr.sin_family = AF_INET;
+ addr.sin_port = htons(port);
+ addr.sin_addr.s_addr = inet_addr("127.0.0.1");
+
+ req = async_connect_send(ev, ev, fd,
+ (struct sockaddr *)&addr,
+ sizeof(struct sockaddr_in),
+ NULL, NULL, NULL);
+
+ if (!tevent_req_poll(req, ev)) {
+ perror("tevent_req_poll() failed");
+ return 1;
+ }
+
+ status = 0;
+ result = async_connect_recv(req, &status);
+ if (result != 0) {
+ return status;
+ }
+ return 0;
+ }
+
+ result = waitpid(pid, &status, 0);
+ if (result == -1) {
+ perror("waitpid");
+ return 1;
+ }
+
+ if (!WIFEXITED(status)) {
+ printf("child status: %d\n", status);
+ return 2;
+ }
+
+ exit_status = WEXITSTATUS(status);
+ printf("test done: status=%d\n", exit_status);
+
+ if (exit_status != 0) {
+ return exit_status;
+ }
+
+ return 0;
+}
diff --git a/lib/async_req/wscript_build b/lib/async_req/wscript_build
index e8af569..9c25223 100644
--- a/lib/async_req/wscript_build
+++ b/lib/async_req/wscript_build
@@ -7,3 +7,7 @@ bld.SAMBA_SUBSYSTEM('LIBASYNC_REQ',
deps='tevent-util socket-blocking'
)
+bld.SAMBA_BINARY('async_connect_send_test',
+ source='async_connect_send_test.c',
+ deps='LIBASYNC_REQ'
+)
diff --git a/source3/script/tests/test_async_req.sh b/source3/script/tests/test_async_req.sh
new file mode 100755
index 0000000..a92f990
--- /dev/null
+++ b/source3/script/tests/test_async_req.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+
+incdir=`dirname $0`/../../../testprogs/blackbox
+. $incdir/subunit.sh
+
+failed=0
+
+testit "async_connect_send" $VALGRIND $BINDIR/async_connect_send_test ||
+ failed=`expr $failed + 1`
+
+testok $0 $failed
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index ecbb368..4f6c123 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -249,6 +249,9 @@ plantestsuite(
"samba3.pthreadpool", "nt4_dc",
[os.path.join(samba3srcdir, "script/tests/test_pthreadpool.sh")])
+plantestsuite("samba3.async_req", "nt4_dc",
+ [os.path.join(samba3srcdir, "script/tests/test_async_req.sh")])
+
#smbtorture4 tests
base = ["base.attr", "base.charset", "base.chkpath", "base.defer_open", "base.delaywrite", "base.delete",
--
2.1.0
From b394e814c74f290be460b6639c5e0f7bac3532ab Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sun, 18 Oct 2015 22:21:10 +0200
Subject: [PATCH 2/2] async_req: fix non-blocking connect()
According to Stevens UNIX Network Programming and various other sources,
the correct handling for non-blocking connect() is:
- when the initial connect() return -1/EINPROGRESS polling the socket
for *writeability*
- in the poll handler call getsocktopt() with SO_ERROR to get the
finished connect() return value
Simply calling connect() a second time without error checking is
probably wrong and not portable. For a successfull connect() Linux
returns 0, but Solaris will return EISCONN:
24254: 0.0336 0.0002 connect(4, 0xFEFFECAC, 16, SOV_DEFAULT) Err#150 EINPROGRESS
24254: AF_INET name = 10.10.10.143 port = 1024
24254: 0.0349 0.0001 port_associate(3, 4, 0x00000004, 0x0000001D,0x080648A8) = 0
24254: 0.0495 0.0146 port_getn(3, 0xFEFFEB50, 1, 1, 0xFEFFEB60) = 1 [0]
24254: 0.0497 0.0002 connect(4, 0x080646E4, 16, SOV_DEFAULT) Err#133 EISCONN
24254: AF_INET name = 10.10.10.143 port = 1024
Bug: https://bugzilla.samba.org/show_bug.cgi?id=11564
Signed-off-by: Ralph Boehme <slow at samba.org>
---
lib/async_req/async_sock.c | 56 ++++++++++++++++++++++------------------------
1 file changed, 27 insertions(+), 29 deletions(-)
diff --git a/lib/async_req/async_sock.c b/lib/async_req/async_sock.c
index bc3780c..c0ad8f3 100644
--- a/lib/async_req/async_sock.c
+++ b/lib/async_req/async_sock.c
@@ -127,24 +127,17 @@ struct tevent_req *async_connect_send(
return tevent_req_post(req, ev);
}
- /**
- * A number of error messages show that something good is progressing
- * and that we have to wait for readability.
- *
- * If none of them are present, bail out.
+ /*
+ * The only errno indicating that the connect is still in
+ * flight is EINPROGRESS, everything else is an error
*/
- if (!(errno == EINPROGRESS || errno == EALREADY ||
-#ifdef EISCONN
- errno == EISCONN ||
-#endif
- errno == EAGAIN || errno == EINTR)) {
+ if (errno != EINPROGRESS) {
tevent_req_error(req, errno);
return tevent_req_post(req, ev);
}
- state->fde = tevent_add_fd(ev, state, fd,
- TEVENT_FD_READ | TEVENT_FD_WRITE,
+ state->fde = tevent_add_fd(ev, state, fd, TEVENT_FD_WRITE,
async_connect_connected, req);
if (state->fde == NULL) {
tevent_req_error(req, ENOMEM);
@@ -189,27 +182,32 @@ static void async_connect_connected(struct tevent_context *ev,
struct async_connect_state *state =
tevent_req_data(req, struct async_connect_state);
int ret;
-
- if (state->before_connect != NULL) {
- state->before_connect(state->private_data);
- }
-
- ret = connect(state->fd, (struct sockaddr *)(void *)&state->address,
- state->address_len);
-
- if (state->after_connect != NULL) {
- state->after_connect(state->private_data);
- }
-
- if (ret == 0) {
- tevent_req_done(req);
+ int socket_error = 0;
+ socklen_t slen = sizeof(socket_error);
+
+ ret = getsockopt(state->fd, SOL_SOCKET, SO_ERROR,
+ &socket_error, &slen);
+
+ if (ret != 0) {
+ /*
+ * According to Stevens this is the Solaris behaviour
+ * in case the connection encountered an error:
+ * getsockopt() fails, error is in errno
+ */
+ tevent_req_error(req, errno);
return;
}
- if (errno == EINPROGRESS) {
- /* Try again later, leave the fde around */
+
+ if (socket_error != 0) {
+ /*
+ * Berkeley derived implementations (including) Linux
+ * return the pending error via socket_error.
+ */
+ tevent_req_error(req, socket_error);
return;
}
- tevent_req_error(req, errno);
+
+ tevent_req_done(req);
return;
}
--
2.1.0
More information about the samba-technical
mailing list