On Wed, Mar 02, 2016 at 05:51:47PM -0500, Simo Sorce wrote:
See ticket #2968.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From dcaae5431617312b69d175274c8b29c430ec6b04 Mon Sep 17 00:00:00
2001
From: Simo Sorce <simo(a)redhat.com>
Date: Wed, 2 Mar 2016 14:33:38 -0500
Subject: [PATCH 1/3] Util: Move socket setup in a common utility file
Other components may need to connect sockets, the code here is generic enough
that with minimal modifications can be used for non-ldap connections too.
So create a sss_sockets.c/h utility file with all the non-ldap specific socket
setup functions and make them available for other uses.
You need to add sss_sockets.h to noinst_HEADERS, otherwise distcheck
will fail.
And one more nitpick..
+struct tevent_req *sssd_async_socket_init_send(TALLOC_CTX *mem_ctx,
+ struct tevent_context *ev,
+ struct sockaddr_storage *addr,
+ socklen_t addr_len, int timeout)
+{
+ struct sssd_async_socket_state *state;
+ struct tevent_req *req, *subreq;
+ struct timeval tv;
+ int ret;
+
+ req = tevent_req_create(mem_ctx, &state, struct sssd_async_socket_state);
+ if (req == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create failed.\n");
+ return NULL;
+ }
+ state->sd = -1;
+
+ talloc_set_destructor((TALLOC_CTX *)state,
+ sssd_async_socket_state_destructor);
+
+ state->sd = socket(addr->ss_family, SOCK_STREAM, 0);
+ if (state->sd == -1) {
+ ret = errno;
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "socket failed [%d][%s].\n", ret, strerror(ret));
+ goto fail;
+ }
+
+ ret = set_fd_flags_and_opts(state->sd);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "set_fd_flags_and_opts failed.\n");
+ goto fail;
+ }
+
+ DEBUG(SSSDBG_TRACE_ALL,
+ "Using file descriptor [%d] for LDAP connection.\n", state->sd);
You can drop "LDAP" here :)
From bdc65ef92a8dd7bf7542c76cdb838c221c92437a Mon Sep 17 00:00:00
2001
From: Simo Sorce <simo(a)redhat.com>
Date: Wed, 2 Mar 2016 15:49:27 -0500
Subject: [PATCH 2/3] Util: Set socket options and flags separately
Reorganize functions to set options and flags, all flags can be set at once,
and there is no need to keep old falgs around as nothing ever used that for
anything useful.
One nitpick and one question..
Related:
https://fedorahosted.org/sssd/ticket/2968
---
src/util/sss_sockets.c | 58 +++++++++++++++++++-------------------------------
1 file changed, 22 insertions(+), 36 deletions(-)
diff --git a/src/util/sss_sockets.c b/src/util/sss_sockets.c
index d0283af2d18ad42a13ec00faf7d6c4934eb696d0..9be9f2edb5f21dad331cc6a707acacf16692dd24
100644
--- a/src/util/sss_sockets.c
+++ b/src/util/sss_sockets.c
@@ -32,28 +32,35 @@
#include "util/util.h"
-static errno_t set_fd_flags_and_opts(int fd)
+static errno_t set_fd_flags(int fd, long flags)
{
int ret;
- long flags;
- int dummy = 1;
+ long cur_flags;
- flags = fcntl(fd, F_GETFD, 0);
- if (flags == -1) {
+ cur_flags = fcntl(fd, F_GETFD, 0);
+ if (cur_flags == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
"fcntl F_GETFD failed [%d][%s].\n", ret, strerror(ret));
return ret;
}
- flags = fcntl(fd, F_SETFD, flags| FD_CLOEXEC);
- if (flags == -1) {
+ cur_flags = fcntl(fd, F_SETFD, cur_flags | flags);
This is the nitpick, can you assign to ret here, F_SETFD doesn't return
flags but the code reads as if it did..
+ if (cur_flags == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
"fcntl F_SETFD failed [%d][%s].\n", ret, strerror(ret));
return ret;
}
+ return EOK;
+}
+
+static errno_t set_fd_common_opts(int fd)
+{
+ int dummy = 1;
+ int ret;
+
/* SO_KEEPALIVE and TCP_NODELAY are set by OpenLDAP client libraries but
* failures are ignored.*/
ret = setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, &dummy, sizeof(dummy));
[...]
@@ -190,11 +175,6 @@ static void sssd_async_connect_done(struct
tevent_context *ev,
talloc_zfree(fde);
- fret = fcntl(state->fd, F_SETFL, state->old_flags);
- if (fret != EOK) {
- DEBUG(SSSDBG_CRIT_FAILURE, "fcntl F_SETFL failed.\n");
- }
-
And this is the question. The new code doesn't restore the flags, is
this an intentional change? Do you know why we restored the flags
previously?
if (ret == EOK) {
tevent_req_done(req);
} else {
From c266d7bde35483305725d0fb23c1b2699848c75c Mon Sep 17 00:00:00
2001
From: Simo Sorce <simo(a)redhat.com>
Date: Wed, 2 Mar 2016 17:38:10 -0500
Subject: [PATCH 3/3] Util Sockets: Tidy up connect() handling
The connect() man page says waiting on a non-blocking connect should be done
by checking for writability, so drop checking for readability. Also check
for EALREADY as an acceptable error to retry on.
ACK