Having problems understanding async_connect_send and async_connect_connected in lib/async_req/async_soc.c
Jeremy Allison
jra at samba.org
Tue Oct 20 17:49:22 UTC 2015
On Tue, Oct 20, 2015 at 05:44:30PM +0200, Ralph Boehme wrote:
>
> 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.
Yeah, I think we do !
However the test has a problem (in theory if not in practice :-).
port must be uint16_t, and should go up to UINT16_MAX,
not INT32_MAX.
> Please review carefully and push if ok. Thanks!
Modified version attached. Push if you're happy !
Jeremy.
-------------- next part --------------
From 1ef63ce44bde0546aa88d406bc17d7edaba8b322 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>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
lib/async_req/async_connect_send_test.c | 130 ++++++++++++++++++++++++++++++++
lib/async_req/wscript_build | 4 +
source3/script/tests/test_async_req.sh | 11 +++
source3/selftest/tests.py | 3 +
4 files changed, 148 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..e612056
--- /dev/null
+++ b/lib/async_req/async_connect_send_test.c
@@ -0,0 +1,130 @@
+/*
+ * 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;
+ uint16_t 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 < UINT16_MAX; port++) {
+ addr.sin_port = htons(port);
+ result = bind(listen_sock, (struct sockaddr *)&addr, sizeof(addr));
+ if (result == 0) {
+ break;
+ }
+ }
+
+ if (port == UINT16_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.6.0.rc2.230.g3dd15c0
From ddacf98a4a16b8f7c1d0a0689c7ebb926b0ab34f 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>
Reviewed-by: Jeremy Allison <jra 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.6.0.rc2.230.g3dd15c0
More information about the samba-technical
mailing list