[SSSD] Re: [PATCH] make async connect generic

2016-03-05 Thread Jakub Hrozek 
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 
> 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 
> 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 
> Date: Wed, 2 Mar 2016 17:38:10 -0500
> Subject: [PATCH 3/3] Util Sockets: Tidy up con

[SSSD] Re: [PATCH] make async connect generic

2016-03-08 Thread Simo Sorce 
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 9b8fd65b6eb242936a5d0734eb05e3c09d3268a5 Mon Sep 17 00:00:00 2001
From: Simo Sorce 
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.

Resolves:
https://fedorahosted.org/sssd/ticket/2968
---
 Makefile.am|   5 +
 src/util/sss_ldap.c| 258 ++-
 src/util/sss_sockets.c | 356 +
 src/util/sss_sockets.h |  39 ++
 4 files changed, 413 insertions(+), 245 deletions(-)
 create mode 100644 src/util/sss_sockets.c
 create mode 100644 src/util/sss_sockets.h

diff --git a/Makefile.am b/Makefile.am
index 4e4f38a5eaf5bfa2cfafa88b9b32848e6c27131b..d6eb0fc732a3b566dba91952bd6598a31f8d8d3e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -541,6 +541,7 @@ dist_noinst_HEADERS = \
 src/util/sss_python.h \
 src/util/sss_krb5.h \
 src/util/sss_selinux.h \
+src/util/sss_sockets.h \
 src/util/sss_utf8.h \
 src/util/sss_ssh.h \
 src/util/sss_ini.h \
@@ -1663,6 +1664,7 @@ ipa_ldap_opt_tests_SOURCES = \
 src/providers/ad/ad_opts.c \
 src/providers/ipa/ipa_opts.c \
 src/providers/krb5/krb5_opts.c \
+src/util/sss_sockets.c \
 src/util/sss_ldap.c \
 src/tests/ipa_ldap_opt-tests.c
 ipa_ldap_opt_tests_CFLAGS = \
@@ -1877,6 +1879,7 @@ TEST_MOCK_RESP_OBJ = \
  src/responder/common/responder_cache_req.c
 
 TEST_MOCK_PROVIDER_OBJ = \
+ src/util/sss_sockets.c \
  src/util/sss_ldap.c \
  src/providers/data_provider_opts.c \
  src/providers/ldap/ldap_opts.c \
@@ -2264,6 +2267,7 @@ sdap_tests_SOURCES = \
 src/providers/ldap/sdap_range.c \
 src/providers/ldap/ldap_opts.c \
 src/providers/ipa/ipa_opts.c \
+src/util/sss_sockets.c \
 src/util/sss_ldap.c \
 src/tests/cmocka/test_sdap.c \
 $(NULL)
@@ -2879,6 +2883,7 @@ libsss_ldap_common_la_SOURCES = \
 src/providers/ldap/sdap.c \
 src/providers/ipa/ipa_dn.c \
 src/util/user_info_msg.c \
+src/util/sss_sockets.c \
 src/util/sss_ldap.c \
 $(NULL)
 libsss_ldap_common_la_CFLAGS = \
diff --git a/src/util/sss_ldap.c b/src/util/sss_ldap.c
index c440d5445133c8b31f662fc69b35e2296c3f1702..7fdaadb5cebf5d3e7fe7f8fb1780f0db3dbcae4a 100644
--- a/src/util/sss_ldap.c
+++ b/src/util/sss_ldap.c
@@ -17,18 +17,13 @@
 You should have received a copy of the GNU General Public License
 along with this program.  If not, see .
 */
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
 
 #include "config.h"
-
-#include "providers/ldap/sdap.h"
-#include "util/sss_ldap.h"
 #include "util/util.h"
+#include "util/sss_sockets.h"
+#include "util/sss_ldap.h"
+
+#include "providers/ldap/sdap.h"
 
 const char* sss_ldap_err2string(int err)
 {
@@ -103,183 +98,6 @@ int sss_ldap_control_create(const char *oid, int iscritical,
 }
 
 #ifdef HAVE_LDAP_INIT_FD
-struct sdap_async_sys_connect_state {
-long old_flags;
-struct tevent_fd *fde;
-int fd;
-socklen_t addr_len;
-struct sockaddr_storage addr;
-};
-
-static void sdap_async_sys_connect_done(struct tevent_context *ev,
-struct tevent_fd *fde, uint16_t flags,
-void *priv);
-
-static struct tevent_req *sdap_async_sys_connect_send(TALLOC_CTX *mem_ctx,
-struct tevent_context *ev,
-int fd,
-const struct sockaddr *addr,
-socklen_t addr_len)
-{
-struct tevent_req *req;
-struct sdap_async_sys_connect_state *state;
-long flags;
-int ret;
-int fret;
-
-flags = fcntl(fd, F_GETFL, 0);
-if (flags == -1) {
-DEBUG(SSSDBG_CRIT_FAILURE, "fcntl F_GETFL failed.\n");
-return NULL;
-}
-
-req = tevent_req_create(mem_ctx, &state,
-struct sdap_async_sys_connect_state);
-if (req ==

[SSSD] Re: [PATCH] make async connect generic

2016-03-08 Thread Jakub Hrozek 
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 
> 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.
> 
> Related:
> https://fedorahosted.org/sssd/ticket/2968

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
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] make async connect generic

2016-03-08 Thread Simo Sorce 
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 
> > 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.
> > 
> > Related:
> > https://fedorahosted.org/sssd/ticket/2968
> 
> 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.

-- 
Simo Sorce * Red Hat, Inc * New York
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] make async connect generic

2016-03-08 Thread Simo Sorce 
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 
> > > 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.
> > > 
> > > Related:
> > > https://fedorahosted.org/sssd/ticket/2968
> > 
> > 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.

-- 
Simo Sorce * Red Hat, Inc * New York
From 6b3c7483793d53dad7b0a629a16d30add21659b5 Mon Sep 17 00:00:00 2001
From: Simo Sorce 
Date: Wed, 2 Mar 2016 15:49:27 -0500
Subject: [PATCH 2/2] 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.

Related:
https://fedorahosted.org/sssd/ticket/2968
---
 src/util/sss_sockets.c | 100 +
 1 file changed, 52 insertions(+), 48 deletions(-)

diff --git a/src/util/sss_sockets.c b/src/util/sss_sockets.c
index d0283af2d18ad42a13ec00faf7d6c4934eb696d0..4ca0029df1057107140590e29f2c5d7804f12a69 100644
--- a/src/util/sss_sockets.c
+++ b/src/util/sss_sockets.c
@@ -32,27 +32,52 @@
 #include "util/util.h"
 
 
-static errno_t set_fd_flags_and_opts(int fd)
+static errno_t set_fcntl_flags(int fd, int fd_flags, int fl_flags)
 {
 int ret;
-long flags;
+int cur_flags;
+
+ret = fcntl(fd, F_GETFD, 0);
+if (ret == -1) {
+ret = errno;
+DEBUG(SSSDBG_CRIT_FAILURE,
+  "fcntl F_GETFD failed [%d][%s].\n", ret, strerror(ret));
+return ret;
+}
+cur_flags = ret;
+
+ret = fcntl(fd, F_SETFD, cur_flags | fd_flags);
+if (ret == -1) {
+ret = errno;
+DEBUG(SSSDBG_CRIT_FAILURE,
+  "fcntl F_SETFD failed [%d][%s].\n", ret, strerror(ret));
+return ret;
+}
+
+ret = fcntl(fd, F_GETFL, 0);
+if (ret == -1) {
+ret = errno;
+DEBUG(SSSDBG_CRIT_FAILURE,
+  "fcntl F_GETFD failed [%d][%s].\n", ret, strerror(ret));
+return ret;
+}
+cur_flags = ret;
+
+ret = fcntl(fd, F_SETFL, cur_flags | fl_flags);
+if (ret == -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;
-
-flags = fcntl(fd, F_GETFD, 0);
-if (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) {
-ret = errno;
-DEBUG(SSSDBG_CRIT_FAILURE,
-  "fcntl F_SETFD failed [%d][%s].\n", ret, strerror(ret));
-return ret;
-}
+int ret;
 
 /* SO_KEEPALIVE and TCP_NODELAY are set by OpenLDAP client libraries but
  * failures are ignored.*/
@@ -77,7 +102,6 @@ static errno_t set_fd_flags_and_opts(int fd)
 
 
 struct sssd_async_connect_state {
-long old_flags;
 struct tevent_fd *fde;
 int fd;
 socklen_t addr_len;
@@ -96,15 +120,7 @@ struct tevent_req *sssd_async_connect_send(TALLOC_CTX *mem_ctx,
 {
 struct teve

[SSSD] Re: [PATCH] make async connect generic

2016-03-09 Thread Jakub Hrozek 
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 
> > > > 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.
> > > > 
> > > > Related:
> > > > https://fedorahosted.org/sssd/ticket/2968
> > > 
> > > 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/
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] make async connect generic

2016-03-09 Thread Jakub Hrozek 
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 
> > > > > 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.
> > > > > 
> > > > > Related:
> > > > > https://fedorahosted.org/sssd/ticket/2968
> > > > 
> > > > 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 mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org