See ticket #2968.
Simo.
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@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@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@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
Fixing everything else commented before.
On Sat, 2016-03-05 at 15:31 +0100, Jakub Hrozek wrote:
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?
Yes, it is an intentional change as restoring the flags was not needed. What happens if the function fails is that we are going to close the socket anyway, so what's the point of restoring flags (which means removing O_NONBLOCK in the end, somethign we never want to do as all sockets must be non-blocking in SSSD to avoig hangs.
Fixed patches attacched.
Simo.
On Tue, Mar 08, 2016 at 10:18:46AM -0500, Simo Sorce wrote:
Fixing everything else commented before.
On Sat, 2016-03-05 at 15:31 +0100, Jakub Hrozek wrote:
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?
Yes, it is an intentional change as restoring the flags was not needed. What happens if the function fails is that we are going to close the socket anyway, so what's the point of restoring flags (which means removing O_NONBLOCK in the end, somethign we never want to do as all sockets must be non-blocking in SSSD to avoig hangs.
Fixed patches attacched.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 5551dc918890cf445cadb1b39c42d9a6dffa8bb8 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@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.
This patch breaks failover for me. I can't really figure out why, the code looks OK, though.
What I'm seeing is that the connect() call blocks. If I just add another SETFD with O_NONBLOCK, everything works fine.
I really don't see the error though, can you? I can reliably reproduce the error with: - search for a user to establish connection - pause the VM with the server - search again
On Tue, 2016-03-08 at 17:48 +0100, Jakub Hrozek wrote:
On Tue, Mar 08, 2016 at 10:18:46AM -0500, Simo Sorce wrote:
Fixing everything else commented before.
On Sat, 2016-03-05 at 15:31 +0100, Jakub Hrozek wrote:
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?
Yes, it is an intentional change as restoring the flags was not needed. What happens if the function fails is that we are going to close the socket anyway, so what's the point of restoring flags (which means removing O_NONBLOCK in the end, somethign we never want to do as all sockets must be non-blocking in SSSD to avoig hangs.
Fixed patches attacched.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 5551dc918890cf445cadb1b39c42d9a6dffa8bb8 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@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.
This patch breaks failover for me. I can't really figure out why, the code looks OK, though.
What I'm seeing is that the connect() call blocks. If I just add another SETFD with O_NONBLOCK, everything works fine.
I really don't see the error though, can you? I can reliably reproduce the error with: - search for a user to establish connection - pause the VM with the server - search again
My bad, I see the error, proper patch coming soon.
Simo.
On Tue, 2016-03-08 at 12:11 -0500, Simo Sorce wrote:
On Tue, 2016-03-08 at 17:48 +0100, Jakub Hrozek wrote:
On Tue, Mar 08, 2016 at 10:18:46AM -0500, Simo Sorce wrote:
Fixing everything else commented before.
On Sat, 2016-03-05 at 15:31 +0100, Jakub Hrozek wrote:
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?
Yes, it is an intentional change as restoring the flags was not needed. What happens if the function fails is that we are going to close the socket anyway, so what's the point of restoring flags (which means removing O_NONBLOCK in the end, somethign we never want to do as all sockets must be non-blocking in SSSD to avoig hangs.
Fixed patches attacched.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 5551dc918890cf445cadb1b39c42d9a6dffa8bb8 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@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.
This patch breaks failover for me. I can't really figure out why, the code looks OK, though.
What I'm seeing is that the connect() call blocks. If I just add another SETFD with O_NONBLOCK, everything works fine.
I really don't see the error though, can you? I can reliably reproduce the error with: - search for a user to establish connection - pause the VM with the server - search again
My bad, I see the error, proper patch coming soon.
Simo.
Please see attached patch, the code path had been compressed .. a lit'l too much :)
Simo.
On Tue, Mar 08, 2016 at 12:28:55PM -0500, Simo Sorce wrote:
On Tue, 2016-03-08 at 12:11 -0500, Simo Sorce wrote:
On Tue, 2016-03-08 at 17:48 +0100, Jakub Hrozek wrote:
On Tue, Mar 08, 2016 at 10:18:46AM -0500, Simo Sorce wrote:
Fixing everything else commented before.
On Sat, 2016-03-05 at 15:31 +0100, Jakub Hrozek wrote:
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?
Yes, it is an intentional change as restoring the flags was not needed. What happens if the function fails is that we are going to close the socket anyway, so what's the point of restoring flags (which means removing O_NONBLOCK in the end, somethign we never want to do as all sockets must be non-blocking in SSSD to avoig hangs.
Fixed patches attacched.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 5551dc918890cf445cadb1b39c42d9a6dffa8bb8 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@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.
This patch breaks failover for me. I can't really figure out why, the code looks OK, though.
What I'm seeing is that the connect() call blocks. If I just add another SETFD with O_NONBLOCK, everything works fine.
I really don't see the error though, can you? I can reliably reproduce the error with: - search for a user to establish connection - pause the VM with the server - search again
My bad, I see the error, proper patch coming soon.
Simo.
Please see attached patch, the code path had been compressed .. a lit'l too much :)
ACK
CI: http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/3873/
On Wed, Mar 09, 2016 at 06:28:50PM +0100, Jakub Hrozek wrote:
On Tue, Mar 08, 2016 at 12:28:55PM -0500, Simo Sorce wrote:
On Tue, 2016-03-08 at 12:11 -0500, Simo Sorce wrote:
On Tue, 2016-03-08 at 17:48 +0100, Jakub Hrozek wrote:
On Tue, Mar 08, 2016 at 10:18:46AM -0500, Simo Sorce wrote:
Fixing everything else commented before.
On Sat, 2016-03-05 at 15:31 +0100, Jakub Hrozek wrote:
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?
Yes, it is an intentional change as restoring the flags was not needed. What happens if the function fails is that we are going to close the socket anyway, so what's the point of restoring flags (which means removing O_NONBLOCK in the end, somethign we never want to do as all sockets must be non-blocking in SSSD to avoig hangs.
Fixed patches attacched.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 5551dc918890cf445cadb1b39c42d9a6dffa8bb8 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@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.
This patch breaks failover for me. I can't really figure out why, the code looks OK, though.
What I'm seeing is that the connect() call blocks. If I just add another SETFD with O_NONBLOCK, everything works fine.
I really don't see the error though, can you? I can reliably reproduce the error with: - search for a user to establish connection - pause the VM with the server - search again
My bad, I see the error, proper patch coming soon.
Simo.
Please see attached patch, the code path had been compressed .. a lit'l too much :)
ACK
CI: http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/3873/
master: * 5dbf360f2d6b0281c32f1bba6ebf5cc834c1716e * 75e66c388862a4ba05afe0791c5503226395bad0 * e05d3f5872263aadfbc2f6a2a8c9735219922387
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Simo Sorce